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
Analysis notebook for tie vs danish #9
base: develop
Are you sure you want to change the base?
Analysis notebook for tie vs danish #9
Changes from 9 commits
b99cfa4
973e47e
6d4743b
cc077ac
6297f5b
dbe7ad8
7fcd0a8
4fb7992
26230f2
73a844c
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.
Given that the
ts_aos_analysis
notebook does not have outputs, it may be a good idea ( beyond attaching PDF of the executed notebook in the PR, and in the ticket) to describe the result here in words (eg. for Z7 Danish result of .. and TIE of ... with estimated uncertainty... agree with manual results ....) . But it is hard to describe results without any pictures, and with fleeting persistency of repos / collections / packages, I wonder if we shouldn't have a repo where these notebooks are actually executed.. (ts_aos_analysis_exec
) ?Perhaps rewrite to avoid tautology, eg.
Note we employ means of combined Zernike estimates from each of the 9 detectors
Reply via ReviewNB
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.
Bryce pointed out that we can't post executed notebooks anywhere public because many of the them contain embargoed data (hence why he deleted the PDFs in the PRs). We have created #aos-notebook-review on Slack to make the review process easier, but I agree we might want somewhere to store the executed notebooks. A private Github repo could work, but the size of the history will quickly get cumbersome. Maybe instead we create a shared Google drive folder where we can mirror the directory structure of the repo and drop PDFs of executed notebooks? It's a little cumbersome, but we could make adding the PDF to the drive be the last stage of the PR process?
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.
Yes, the slack channel is not a bad idea, but it may end up requiring too much scrolling with "v1", "better v1", "better v1 after comments" .... If we can create a shared private google drive (or some other approved location) that would work better (in reply to review one can just overwrite).
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 the place to have that is probably JIRA in the different tickets, also for traceability in the future for the project, it would be better than an ad-hoc solution that only AOS people can access
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.
@gmegh we cannot put compiled PDFs in tickets because they frequently contain on-sky pixels
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.
typo "mostly gree".
And I think it strengthens the text by putting a numerical value ("noisiest, with 150nm rms"), and later about disagreements (~200 nm difference for Z5, with amplitude of ~1200 nm is ~16% , and for Z13,Z20...)
Reply via ReviewNB
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.
Done