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

Move sanity check of inputs within argument-parse function to fail fast #34

Open
atteggiani opened this issue Aug 12, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request priority:medium

Comments

@atteggiani
Copy link
Collaborator

In the code there are several lines in which sanity check of inputs take place.
For example in the bounding_box class:

if Path(maskfname).exists():
d = iris.load(maskfname, var)
d = d[0]
d = xr.DataArray.from_iris(d)
lons = d['longitude'].data
lonmin = np.min(lons)
lonmax = np.max(lons)
if lonmax > 180.:
lonmax = lonmax-360.
lats = d['latitude'].data
latmin = np.min(lats)
latmax = np.max(lats)
d.close()
else:
print(f'ERROR: File {maskfname} not found', file=sys.stderr)
raise

or

if Path(ncfname).exists():
d = xr.open_dataset(ncfname)
else:
print(f'ERROR: File {ncfname} not found', file=sys.stderr)
sys.exit(1)

Since the inputs for the class come from command line arguments (parsed through argparse in the hres_ic.py), it would be best to move all input argument's sanity check within a "parse_argument" function to:

  • fail fast (at the beginning of the parent script)
  • improve testing
  • clear up code and group similar functionality all in one place
@atteggiani atteggiani moved this to Todo ⏳ in ACCESS-RAM3 Aug 12, 2024
@atteggiani atteggiani self-assigned this Aug 12, 2024
@aidanheerdegen
Copy link
Member

Seems lowish priority. I've made this a medium priority in case it we would end up writing tests that would then have to be rewritten after this fairly straightforward change.

@atteggiani
Copy link
Collaborator Author

In which Project board did you make it a medium priority? I couldn't see any priority assigned to it so in the ACCESS-RAM3 Project I assigned a Low priority.
Please feel free to change it as you prefer.

Anyway, I agree this is a low priority issue.
On the unit-testing matter: yes, it would definitely require changes with the fixing of this issue, however I don't expect the changes to be very big, so it should be completely feasible to fix this issue even if unit-testing is already present in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:medium
Projects
Status: Todo ⏳
Development

No branches or pull requests

2 participants