Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JP-3502: Extend snowball flags to next integration #238
JP-3502: Extend snowball flags to next integration #238
Changes from 14 commits
6fbeb56
85df783
6da60a8
79128ae
33f63b4
1a28914
bc20f4d
605f845
44cb702
b36bdb5
fbb0d5b
cc23831
c9cacac
4bb6455
57d8fb3
1a3319a
46f96e4
b75dd06
9d5df58
0ba61ac
8f3dcb4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function now has roughly 30 arguments. Would it be better served by using a class to contain the parameters, rather than continue to add parameters to this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it has a lot of parameters but users need to be able to tweak the parameters. Is there any cost to the large number of parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cost is readability and maintainability. Grouping various parameters into a class or classes makes the code easier to read, as well as easier to maintain, especially when adding a parameter that needs to be passed to subroutines. If you simply pass parameters, the function signature needs to be changed for all functions that need that parameter. If you use classes, those classes will be passed to the subroutines, so when you add a parameter to a class, no subroutine signature needs to be edited, since the parameter is contained in the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it is true that passing objects instead of primitives has advantages there are also disadvantages. APIs with only primitive parameters are easier to understand where the primitives are coming from. At the extreme, a function that has a single class as input in a black box. Someone browsing the code has more work to do than with primitive parameters.
Also, If we want to consolidate the parameters passed to detect_jump.py, I feel that the change should be in a separate PR. That would give us a working master to compare against.
What do you think?
Check warning on line 605 in src/stcal/jump/jump.py
Codecov / codecov/patch
src/stcal/jump/jump.py#L603-L605
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is special about 22 here? A comment or declaring a descriptive variable with the value 22 would be good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the comments in this section. Let me know, if you think it needs more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't know why you use 22 instead of say 21 or 23.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real reason that I can remember. There needs to be a number that I can use np.where to find the pixels that are affected. I'm sure there's a more elegant way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be fine to make a comment saying this number could change and that this number was determined though trial and error and could be changed if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of 22 is arbitrary. It just needs to be something that can be tested for to create the mask of pixels within the ellipse.
Check warning on line 346 in tests/test_jump.py
Codecov / codecov/patch
tests/test_jump.py#L343-L346