Fixed bug when using plasma_density_from_file #136
Conversation
| # check that there is not already a plasma density profile set | ||
| assert self.plasma_density_from_file is None | ||
| # If there is already a density file open and make the plasma profile | ||
| if self.plasma_density_from_file: |
There was a problem hiding this comment.
Check that self.plasma_density_from_file is type str.
Also catch non-existent file, e.g. raise ValueError with meaningful error message
There was a problem hiding this comment.
Maybe here explicitly check that it is None, otherwise e.g. False (a bool) or 0 (an int) would have the same meaning as None here but maybe not everywhere else.
| # If there is already a density file open and make the plasma profile | ||
| if self.plasma_density_from_file: | ||
| ss, ns = [], [] | ||
| with open(self.plasma_density_from_file, 'r') as f: |
There was a problem hiding this comment.
Catch errors when reading / parsing file
kyrsjo
left a comment
kyrsjo
left a comment
There was a problem hiding this comment.
Error handling, delete wrong comment "Save to file" (first one...)
|
Could you please fix (i.e. delete it) comment here: |
| self.plasma_profile.ns = ns | ||
|
|
||
| return | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
Good job, but this block only handles the case where the file doesn’t exist. If another problem occurs, e.g. permission denied, or the path is a directory, you will get an unhandled exception (like PermissionError) and the program will crash, unless you catch it too.
|
|
||
| return | ||
| except FileNotFoundError: | ||
| raise FileNotFoundError("File can not be located using given path") from None |
There was a problem hiding this comment.
Typo in the error message. Should be "File cannot be located using given path.". Or perhaps it is better to instead write "Could not open specified plasma profile file.", which is more specific regarding what file could not be opened.
There was a problem hiding this comment.
Indeed, something like f"Could not open specified plasma profile file '{self.plasma_density_from_file}'." would probably be ideal.
There was a problem hiding this comment.
Could you please also add a comment in the code about from None
| assert self.plasma_density_from_file is None | ||
| # If there is already a density file open and make the plasma profile | ||
| if self.plasma_density_from_file: | ||
| if isinstance(self.plasma_density_from_file, str): |
There was a problem hiding this comment.
I suggest something like:
try:
data = np.loadtxt(self.plasma_density_from_file, comments='#')
ss, ns = data[:, 0], data[:, 1]
self.plasma_profile.ss = ss
self.plasma_profile.ns = ns
return
except OSError:
raise FileNotFoundError("Plasma profile file cannot be located or opened using given path.") from None
except ValueError:
raise ValueError("Plasma profile file must contain two numeric values (SI units) in the order <longitudinal position> <plasma density> per line.") from NoneThere was a problem hiding this comment.
This avoids unnecessary conversions, loop and is far more compact. Should also be able to handle commented lines and blank lines, as well as handle more errors.
But Please TEST it for me.
There was a problem hiding this comment.
(also please use f-strings to include the bad file name etc.)
|
@finnerudo what is the status of this branch? |
When using plasma_density_from_file to make plasma ramp in HiPACE++, abel would crash because there was an assert self.plasma_density_from_file is None that is executed everytime a HiPACE++ stage is ran. I changed it to if there is plasma_density_from file it sets the plasma profile ss and ns which is needed for tracking the plasma density and just returns.