history updated just if acquisition succeeded#1401
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1401 +/- ##
==========================================
- Coverage 96.79% 96.78% -0.02%
==========================================
Files 133 133
Lines 10463 10468 +5
==========================================
+ Hits 10128 10131 +3
- Misses 335 337 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| platform: Optional[Platform] = None, | ||
| targets: Optional[Targets] = None, | ||
| mode: Optional[ExecutionMode] = None, | ||
| mode: Optional[ExecutionMode] = [], |
There was a problem hiding this comment.
Moreover, the if mode is None you introduced yourself below is already doing the replacement
| # None is no iterable so code might crash in lines | ||
| # 169 and 185 |
There was a problem hiding this comment.
Never explicitly mention lines of code: these comments are going to age fast, and become obsolete and confusing. Mention the location "conceptually", naming their role, rather than the physical position.
| if mode is None: | ||
| # None is no iterable so code might crash later when checking | ||
| # executino modes if-statements since None is not iterable | ||
| mode = [] |
There was a problem hiding this comment.
mode is a Flag enum, specifically of type ExecutionMode.
Instead of making it an empty list, which is succeeding in the current usage, but just broadening the type hint, create instead an instance of ExecutionMode.
If you want to keep it empty as a default (which is a choice), it is possible to create the equivalent enum variant, e.g. with ExecutionMode.ACQUISITION & ExecutionMode.FIT (empty intersection).
It is a bit convoluted, and because of this in py3.11 on you can do ExecutionMode(0). But we still support also py3.10
Btw: the reason not to put an empty list as default argument is because it is a mutable object, and it would be shared by all calls (search this online, you will find plenty of references and examples). However, ExecutionMode instances should not be mutable, because they are essentially integers. So, that one could be set even as default argument, and you could avoid both the Optional and the default to None. Though that's also a choice: sometimes having a special value for the unset state is useful. But, in my experience, better to avoid whenever not explicitly needed
There was a problem hiding this comment.
then what about using a tuple?
There was a problem hiding this comment.
Do you mean a tuple of Booleans? That's exactly what a flag is, essentially a bitstring :)
There was a problem hiding this comment.
I was more thinking about changing the default value to an empty tuple, which then is immutable and will be fine with the immutability condition, on which by the way I completely agree.
The thing is that there is a clear inconsistency in the default value for variable mode, since if you don't assign any value the code will crash because of the command:
if ExecutionMode.Something in None:Then, I don't know if this condition might be forced or is suppressed in higher levels in the code.
Also, the main purpose of this small PR is to put acquisition as necessary and sufficient condition for updating the history in the output folder.
When something crashes in the execution of experiments during the
_fitstep, the program still saves thedata.npzfile so in theory it is still possible to fix the fitting function and test it using the CLI commandqq fit.However it does not work out of the box since in this scenario
history.jsondoes not contain the failed task, hence it ignore those data.In thi PR indeed I try to update this json file with the sufficient and necessary condition that
acquisitionstep succedeed. This solves the scenario described above.Of course this is simply
postponingan eventual crash in the code in the following lines, where indeed theExecutorseeshistory.jsonhas been updated with the task and look forresults.jsoneven though it does not exist.Finally it modifies a small bug that I found in
task.py:runmethod, that from what I saw probably it has never been triggered.