Repository navigation
Conversation
bc34539 to
eda15dc
Compare
72a627d to
e8a494b
Compare
9816a42 to
d4d920c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4523 +/- ##
==========================================
+ Coverage 50.26% 50.32% +0.06%
==========================================
Files 181 181
Lines 30632 30601 -31
==========================================
+ Hits 15396 15399 +3
+ Misses 15236 15202 -34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
756ea41 to
698f4a5
Compare
fec93fb to
bcd063a
Compare
je-cook
left a comment
There was a problem hiding this comment.
First round, I think there will be a few subsequent reviews, its a lot to unpick!
timothy-nunn
left a comment
There was a problem hiding this comment.
A couple more in addition to James.
| if variable_name == "bounds": | ||
| boundl = {} | ||
| boundu = {} | ||
| for index, bounds_dict in info.value.items(): | ||
| try: | ||
| idx = int(index) | ||
| except ValueError as e: | ||
| raise ProcessValidationError( | ||
| "The bounds need to be of the form boundl/u(index) = value" | ||
| ) from e | ||
| if "l" in bounds_dict: | ||
| boundl[idx] = bounds_dict["l"] | ||
| if "u" in bounds_dict: | ||
| boundu[idx] = bounds_dict["u"] | ||
| # TODO could change them into arrays here? | ||
|
|
||
| info_boundl = info | ||
| info_boundl.value = boundl | ||
| info_boundl.name = "boundl" | ||
| variables = set_on_datastructure( | ||
| variable_name="boundl", | ||
| info=info_boundl, | ||
| data_structure_obj=data_structure_obj, | ||
| variables=variables, | ||
| ) | ||
|
|
||
| # matches (variable name, array index, value) | ||
| # NOTE: array index is Fortran-based hence starts at 1. | ||
| line_match = re.match( | ||
| r"([a-zA-Z0-9_]+)(?:\(([0-9]+)\))?[ ]*=[ ]*([ +\-a-zA-Z0-9.,]+).*", | ||
| stripped_line, | ||
| ) | ||
| info_boundu = info | ||
| info_boundu.value = boundu | ||
| info_boundu.name = "boundu" |
There was a problem hiding this comment.
Could this be an additional action, rather than an if statement in the parser?
There was a problem hiding this comment.
Have discussed and decided, for now, to add a new function _handle_bounds - the additional actions are not performed until after the variable validating and cleaning, which means it would be tricky to move this to an additional action and still be able to clean and validate the variable. Can always change once we have decided on how to handle bounds in future
c9765aa to
43c5333
Compare
cbd35eb to
3a44a01
Compare
…undl(index) = value , and use f_nd_impurity_electrons instead of bound in a test for input array remove outdated comment and add TODO docstring to fill in after review
… update any associated mfiles
6788335 to
e2116ac
Compare
There was a problem hiding this comment.
Looks good just the one structural comment. I think we can go further to collating some of these thing under one wrapper.
Move the init.py file into the core/data_structure folder I think. The contents of that file is 90% the check_process. I would rename the file to check_input.py. The other functions in that file can go onto the DataStructure object in data_structure/base.py except for get_git_summary which I would put in as a standalone function in the file. In my head the check_process function will eventually disappear as the validation moves onto the individual data structure objects. Right now we should just isolate it.
All of the functions in input.py can also move to the data structure. I would leave InputVariable and the associated dictionary where it is
There was a problem hiding this comment.
Why is this (and examples/data/large_tokamak_varyrun_IN.DAT) changing?
There was a problem hiding this comment.
because these are duplicate variables that threw an error as this PR raises an error now when there are duplicate vars
| if variable_name == "bounds": | ||
| variables = _handle_bounds(info, data_structure_obj, variables) |
There was a problem hiding this comment.
Sorry I still don't like the custom handling of the bounds with if statements. I have thought of a way of doing this:
At the end of InDat.read_in_dat you should have access to self.data["bounds"]. You could process this dictionary into the respective boundl and boundu arrays and then you wouldn't need all of this custom stuff in the parser because the variables could be parsed essentially as they were before
…s before setting on data structure so as to not overwrite the bounds with unrealistic values
Description
validate_inputto theInDatclass and rename tocheck_obsolete_variablesparse_input_fileno longer re-reads the file line by line and instead uses the data read in byInDatChecklist
I confirm that I have completed the following checks: