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

Clean up help message for configs #153

Closed
wants to merge 3 commits into from
Closed

Clean up help message for configs #153

wants to merge 3 commits into from

Conversation

mhliu0001
Copy link
Contributor

Previously there were inconsistent help for the same configuration in different files. And there were also two configs (pe_pulse_ys and pe_pulse_ts) that had a placeholder description. This PR fixes these descriptions to make them consistent with each other.

Also I will let experts comment on whether my interpretation for pe_pulse_ys and pe_pulse_ts is correct.

@mhliu0001
Copy link
Contributor Author

mhliu0001 commented Mar 6, 2024

Checking this manually is tiresome, so it might be nice to integrate this into the automatic tests in the future. I have a script (by ChatGPT) for this if anyone is interested.

@coveralls
Copy link

coveralls commented Mar 6, 2024

Pull Request Test Coverage Report for Build 8342687699

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 66.592%

Totals Coverage Status
Change from base Build 8339955236: 0.0%
Covered Lines: 1782
Relevant Lines: 2676

💛 - Coveralls

@mhliu0001
Copy link
Contributor Author

Also in s2_photon_propagation.py, many help messages are just the same as the name of the config. I need some help here what these configs are.

@mhliu0001 mhliu0001 added the help wanted Extra attention is needed label Mar 6, 2024
@mhliu0001 mhliu0001 marked this pull request as draft March 6, 2024 19:45
@HenningSE
Copy link
Collaborator

Hi @mhliu0001, thanks for bringing this up. For some of the variables I was not really sure how to best describe them in help. Many variable names were taken 1:1 from WFSim and they are not very easy to decipher. We could even think about replacing variable names like e.g. pe_pulse_ts with something easier to understand while collecting the informations for the help statements.

@mhliu0001 mhliu0001 requested a review from dachengx March 8, 2024 18:55
@ramirezdiego
Copy link
Collaborator

Merged into #163.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants