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

command line "-h" does not provide a correct description of luminosity options #79

Open
itaboada opened this issue Jul 8, 2022 · 1 comment

Comments

@itaboada
Copy link
Collaborator

itaboada commented Jul 8, 2022

This needs to be made consistent with the rewriting of the Luminosity class.

@mlincett
Copy link
Contributor

mlincett commented Jul 12, 2022

After the changes by @ChrisCFTung , the **kwargs received by firesong_simulation() are directly passed to get_LuminosityFunction() and get_evolution() (note that in the latter I have implemented the support for parameterised evolutions, that is why now we pass optional arguments here as well). If I am not wrong we lost the configuration string parsing functionality that was previously suggested.

I agree with Chris that firesong_simulation() should receive numerical values for the LF and evo parameters, rather than a configuration string embedding numbers. However, I think the configuration string would still be the best option for the CLI interface.

My suggestion the following:

  • firesong_simulation() should take two distinct dictionaries to provide the parameters configuring LF and evolution, that are passed to get_LuminosityFunction() and get_evolution() respectively. I see no good reason to pass an unspecified set of keyword args to both methods and let them deal with parameters that are not meant for them;
  • we use configuration strings to provide LF and evolution description to the CLI interface;
  • a small parser will translate the configuration strings to the respective dictionaries.

As an alternative, with argparse it could be possible to pass down all unrecognised CLI arguments to firesong_simulation(), leaving free hand to the user, but I think it would become increasingly complicate to input separately the configuration parameters with the command line.

Of course I am fine with any choice as long there's enough consensus :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants