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

Abort with useful message if users try to use an input parameters no longer supported #611

Closed

Conversation

MaxThevenet
Copy link
Member

@MaxThevenet MaxThevenet commented Oct 25, 2021

This PR proposes to explicitly check if the user specifies some old input parameters, and crash with a meaningful message if it is the case. This is not as stable as backward compatibility, but is also much less constraint in terms of development, and should help the user experience.
Test: using a deprecated input parameter Ade the code crash with proper error message:

amrex::Abort::1::DEPRECATED INPUT ERROR:
Input parameter <plasma species>.density is no longer supported.
Use <plasma species>.density(x,y,z) instead. See documentation for more information. !!!

@MaxThevenet MaxThevenet added the documentation Improvements or additions to documentation label Oct 25, 2021
@MaxThevenet
Copy link
Member Author

@SeverinDiederichs @AlexanderSinn
This PR adds a check for <plasma species>.density only, which was replaced by <plasma species>.density(x,y,z) in PR #605. If you think about any input parameter that you changed recently, could you give the input parameter in this PR discussion? Then I'll modify the PR to account for recently changed input parameters.

@AlexanderSinn
Copy link
Member

I can’t think of any right now, but if this feature is going to be used more in the future I would suggest calling Hipace::BackwardCompatibility() inside Hipace_early_init::Hipace_early_init() instead of the constructor to insure it gets called before the program looks for a potentially missing parameter, which could cause a crash without a helpful error massage.

@MaxThevenet
Copy link
Member Author

Agreed, good point, I'll change that.

@SeverinDiederichs
Copy link
Member

I also don't think that we had any breaking changes in the input files recently except for the plasma.density. Before, I think the last breaking change, it was about plasma elements and plasma names when we added multiple plasmas, but that was really long ago, so let's not include that.

@MaxThevenet
Copy link
Member Author

@AlexanderSinn hm but then the main members (like m_multi_plasma) won't have been initialized. So maybe each class should do these tests as early as possible in their respective constructors, but then it's not centralized in 1 place.

@AlexanderSinn
Copy link
Member

Yes good point, maybe just leave it as is and add a comment warning about the ordering (making sure a missing/wrong parameter wont prematurely crash it).

Or the cleanest solution I can think of is to make a function TestDeprecatedInput (amrex::ParmParse& pp, const char * old_param_name, const char * new_param_name ) that tests if the old input is present and if so aborts with an error message mentioning the new input. This would be called directly before the new input is obtained (queryWithParser(pp, "density(x,y,z)", density_func_str);). This option might not be necessary if <plasma>.density is truly the only deprecated input.

@MaxThevenet
Copy link
Member Author

Hm, that's a good idea, but this only works for input parameters that changed name, we would still need specific tests for input parameters that just don't exist anymore. For instance <plasma name>.parabolic_curvature could IMO be just removed, because it is redundant with density(x,y,z) (or at least it could be made deprecated after some further improvements).

@AlexanderSinn
Copy link
Member

I would put TestDeprecatedInput of <plasma name>.parabolic_curvature before queryWithParser(pp, "density(x,y,z)", density_func_str); as well as this is what replaces it.

@MaxThevenet
Copy link
Member Author

replaced by #612

@MaxThevenet MaxThevenet closed this Nov 2, 2021
@MaxThevenet MaxThevenet deleted the backwardcompatibility branch November 2, 2021 10:34
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants