Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update_info_json method is added to update and add the additional inf… #92

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kzqureshi
Copy link
Contributor

update_info_json method is added to update and add the additional information to the info.json file.

@kzqureshi kzqureshi requested a review from lang-m October 6, 2024 13:26
Copy link
Member

@lang-m lang-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to add start_time and end_time to the class instance (self) as this information is specific to one call of drive, not the lifetime of the class, which we can use for multiple consecutive drives.

I would also like to indicate failed status in the info json.

The changes are not currently reflected in schedule. We have to write the initial info json. We can't add anything about success or runtime as we don't know those.

micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
@lang-m
Copy link
Member

lang-m commented Oct 20, 2024

The missing info.json in schedule is also the reason for the test failure.

@kzqureshi kzqureshi requested a review from lang-m November 5, 2024 14:15
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
micromagneticmodel/driver.py Show resolved Hide resolved
micromagneticmodel/driver.py Outdated Show resolved Hide resolved
@lang-m lang-m linked an issue Nov 6, 2024 that may be closed by this pull request
@lang-m
Copy link
Member

lang-m commented Nov 6, 2024

@samjrholt Short summary: info.json is extended to have a complete start data + time, end date + time, elapsed time, and information about success.

We should deprecate the individual date and time keys in info and remove those after some time.

We also move writing the info.json file from the individual calculators to the base class (all required calculator-specific details are available in the base class).

@lang-m lang-m requested a review from samjrholt November 6, 2024 11:54
lang-m
lang-m previously approved these changes Nov 6, 2024
Copy link
Member

@samjrholt samjrholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lang-m @kzqureshi This looks good, the only other thing that I think could be useful to add to the JSON is the version of ubermag being used. That way, in the future, we know what versions of software (e.g. micromagnetic data) are compatible with the structure of the data/files

micromagneticmodel/driver.py Show resolved Hide resolved
@lang-m
Copy link
Member

lang-m commented Nov 8, 2024

@samjrholt You are right, we should add the ubermag version. Maybe best importlib.metadata.version("ubermag")?

assert "end_time" in info
assert "elapsed_time" in info
assert "success" in info
assert info["success"] is True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert info["success"] is True
assert info["success"]

# assumption: this test runs in under one minute
assert elapsed_seconds < 60
else:
raise TypeError("Unexpected format for elapsed_time")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where/when does this happen (in the tests)?

@lang-m lang-m self-requested a review November 19, 2024 16:03
@lang-m lang-m dismissed their stale review November 19, 2024 16:04

Additional tests added

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.58%. Comparing base (4c3dd04) to head (b93eca7).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
micromagneticmodel/driver.py 91.89% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage   99.16%   98.58%   -0.58%     
==========================================
  Files          30       30              
  Lines         597      637      +40     
==========================================
+ Hits          592      628      +36     
- Misses          5        9       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@kzqureshi
Copy link
Contributor Author

closes
ubermag/micromagneticdata#49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more details to info.json
3 participants