smiths / caseStudies

Case studies of (manual) documentation for scientific computing software
3 stars 2 forks source link

Better Implementation for params.gt in derivedValues.py #82

Closed samm82 closed 6 years ago

samm82 commented 6 years ago

TODO:

https://github.com/smiths/caseStudies/blob/63fee225c72db5ff0e73639eec56fab12bb457c2/CaseStudies/glass/Implementations/Python/Implementation/derivedValues.py#L47-L55

could be:

  # cal glass type factor
    if params.gt in ['AN', 'an']:
        params.gtf = 1
    elif params.gt in ['HS', 'hs']:
        params.gtf = 2
    elif params.gt in ['FT', 'ft']:
        params.gtf = 4
    else:
        print("Wrong glass type entered")

We could also potentially add erroneous capitalization instances (eg. 'aN', 'Hs') to the lists and/or apply the .strip() method to params.gt to account for whitespace errors, however, I'm not sure if this would make it too broad/what the implications are of this change.

smiths commented 6 years ago

I like your proposed changes to the code. Please proceed. Let us hold off on capitalization errors etc. It is an interesting idea, but I'm not sure we want to go in that direction.

If I remember correctly, the test cases for GlassBR are not working right now. We need to fix the test cases so that when the code is refactored as you suggest, we can make sure that we run the regression tests. It is hard to imagine your currently proposed code adding an errors, but in general, we want the confidence that re-running the test cases provides.

Does the Python code for GlassBR use exceptions anywhere? Exceptions are a better design choice than simply printing messages to the terminal.

samm82 commented 6 years ago

Exceptions

Exceptions are used in the following places:

checkConstraints.py

https://github.com/smiths/caseStudies/blob/b89249885d2ce2fdd5ce6c09727a0e2844974d66/CaseStudies/glass/Implementations/Python/Implementation/checkConstraints.py#L8-L20

interp.py

https://github.com/smiths/caseStudies/blob/b89249885d2ce2fdd5ce6c09727a0e2844974d66/CaseStudies/glass/Implementations/Python/Implementation/interp.py#L9-L79

@smiths Should I change the printed error into an exception/start on #81? raise SystemExit("Input Error: Invalid glass type entered")

Also, can this issue be closed, since the only remaining things to do are encapsulated in other issues?

smiths commented 6 years ago

Yes, please replace the print statement with exceptions. I suggest that you use ValueError, rather than SystemExit. We may eventually want to write an exception handler that provides some means to correct the erroneous value. Given that you already have an issue for the exceptions (#81), we can leave the current issue closed.