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

Apply pulse parameters #61

Merged
merged 5 commits into from
May 19, 2023
Merged

Apply pulse parameters #61

merged 5 commits into from
May 19, 2023

Conversation

Sosnowsky
Copy link
Member

No description provided.

Copy link
Member

@gregordecristoforo gregordecristoforo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please update the documentation in the Readme file according to your changes?

blobmodel/blobs.py Show resolved Hide resolved
blobmodel/pulse_shape.py Outdated Show resolved Hide resolved
blobmodel/stochasticality.py Show resolved Hide resolved
blobmodel/stochasticality.py Outdated Show resolved Hide resolved
@Sosnowsky
Copy link
Member Author

I updated the Readme a little bit, not sure how much detail you want in there: I think the code was supposed to still work as it was working before this MR.

Copy link
Member

@gregordecristoforo gregordecristoforo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now the documentation in the README.md is enough. We should, however, implement proper documentation soon. I hope I get to that in the upcoming months.

README.md Show resolved Hide resolved
README.md Outdated
@@ -109,6 +108,8 @@ Alternatively, you can specify all blob parameters exactly as you want by writin
`free_parameter` for vx
- `vy_parameter`: float, optional,
`free_parameter` for vy
- `spx_parameter`: float = 0.5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to change the variable name here. Also, remove trailing ,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is still spx_parameter in the constructor, thought of leaving it short since the others are also shortened (A_dist, etc) and changing them will make old code not work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I have long name for the shape parameter arguments while keeping short name for the others?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's what I've done now anyway

@gregordecristoforo gregordecristoforo merged commit 56803b3 into main May 19, 2023
@gregordecristoforo gregordecristoforo deleted the pulse_params branch May 19, 2023 06:50
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

Successfully merging this pull request may close these issues.

2 participants