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

Case sensitive input parsing #264

Open
jdolence opened this issue Aug 16, 2020 · 12 comments
Open

Case sensitive input parsing #264

jdolence opened this issue Aug 16, 2020 · 12 comments
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed invalid This doesn't seem right question Further information is requested refactor An improvement to existing code.
Projects

Comments

@jdolence
Copy link
Collaborator

Right now, the input file parsing is case sensitive. There are reasonable arguments in favor of this, but it can be a little error prone. For example, with <Parthenon/mesh> instead of <parthenon/mesh>, the code aborts because required mesh parameters aren't found. I just wasted more time than I'd like to admit debugging a problem that turned out to be an incarnation of this basic issue.

I propose we just transform all block and parameter names (but not values on the RHS of the =) to all lowercase internal to Parthenon. This prevents the sort of issue above since <Parthenon/mesh>, <Parthenon/Mesh>, and <PaRtHeNoN/MeSh> are all equivalent, but it potentially introduces a new issue if someone relies on the fact that, for example, important_parameter and Important_Parameter actually set two different fields. I'd argue nobody should be doing that anyway, so I'd suggest we just transform everything to lowercase and document it.

Anyone have an opinion on this? @jmstone does this break compatibility with existing Athena input files?

@jdolence jdolence added documentation Improvements or additions to documentation invalid This doesn't seem right question Further information is requested refactor An improvement to existing code. labels Aug 16, 2020
@Yurlungur
Copy link
Collaborator

I can see a reason for parameter names being case sensitive. I can imagine someone wanting to name a parameter in CamelCase, for example.

However, even if we leave parameter names case sensitive, I think block names should not be.

@jdolence
Copy link
Collaborator Author

@Yurlungur there wouldn't be anything stopping someone from using CamelCase. The trouble would come in if they used CamelCase and camelCase to indicate different parameters because both of these, internal to Parthenon, would become camelcase. But, maybe I haven't understood your comment fully.

@Yurlungur
Copy link
Collaborator

@jdolence hmm that's true. Ignore that objection then.

The other case I can see this mattering (although maybe you shouldn't be doing this) is with parameters or physical constants. G vs. g for Newton's constant and the acceleration due to gravity (or the metric) for example.

@jdolence
Copy link
Collaborator Author

Oh man...G and g. Yeah that's not inconceivable, though I agree you shouldn't do that. Perhaps the thing to do there is to make that a runtime error. If parthenon caught and reported this as an error with a useful message would that cover this case adequately?

@Yurlungur
Copy link
Collaborator

Yeah, I like that. Don't quietly accept it, loudly complain.

@jmstone
Copy link
Collaborator

jmstone commented Aug 17, 2020

There would be no compatibility issues with Athena++ input files. Currently input block names are always all-lowercase (although sometimes users add parameter names that use capitals). Still I don't think will be a serious problem.

One "feature" of the parameter input class that causes me lots of problems is that if you misspell a parameter name, it will read and store the value without complaint. That is because it doesn't know what parameter names are supposed to be and so it can't tell if something is misspelled or not. Changing this would require adding some table of implemented names, and requiring the user to update it if they add anything anywhere (including in the block, which always contains custom values). The latter always seems way too challenging to me, so I just live with it.

@Yurlungur
Copy link
Collaborator

One "feature" of the parameter input class that causes me lots of problems is that if you misspell a parameter name, it will read and store the value without complaint. That is because it doesn't know what parameter names are supposed to be and so it can't tell if something is misspelled or not. Changing this would require adding some table of implemented names, and requiring the user to update it if they add anything anywhere (including in the block, which always contains custom values). The latter always seems way too challenging to me, so I just live with it.

@jmstone does @jdolence's CheckDesired and CheckRequired machinery help with this? I.e., an individual application can declare certain variables it wants to check for their presence? This would allow you to check for mis-spellings for those parameters at least.

@AndrewGaspar
Copy link
Contributor

AndrewGaspar commented Aug 17, 2020

An "easy" fix would be to issue a warning on unused inputs. We could track which inputs are actually read. Then once "initialization" completes, print out the variables that weren't used. That would presumably help people catch their mistake.

EDIT: Nevermind, I just realized we can't do this because we have a hard fail when certain parameters aren't found.

If somebody is feeling particularly ambitious, though, I wouldn't mind it if our input deck was more strictly validated. It would require applications to specify what their inputs are, of course.

@jmstone
Copy link
Collaborator

jmstone commented Aug 18, 2020

Yes, CheckDesired etc. can help in some cases. However, you don't want to issue warnings for parameters that are not required, since in most runs they are not set. The problem comes in the rare cases where you do try to set them, but then accidentally misspell them and the code runs without warning. How I tell the code when and when I do not want it to check for a parameter, and which parameters to check in that case, I am not sure.

@nmsriram
Copy link
Collaborator

I strongly believe that all block names and parameters should be case agnostic. We should not allow ANY instances where two entities are distinguished only by case. If folks want to put in CamelCase or lowerCamelCase in input files to make it more human readable, that is not precluded by this argument.

@JoshuaSBrown
Copy link
Collaborator

JoshuaSBrown commented Aug 18, 2020

I would have to agree with @nmsriram on his point about "not allowing any instances where two entities are distinguished only by case", despite his love for PERL.

@Yurlungur
Copy link
Collaborator

Ping @jdolence open PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed invalid This doesn't seem right question Further information is requested refactor An improvement to existing code.
Projects
Parthenon
  
Awaiting triage
Development

No branches or pull requests

6 participants