Skip to content

Analysis notebook for tie vs danish#9

Open
jfcrenshaw wants to merge 10 commits intodevelopfrom
tickets/SITCOM-1149_20241027
Open

Analysis notebook for tie vs danish#9
jfcrenshaw wants to merge 10 commits intodevelopfrom
tickets/SITCOM-1149_20241027

Conversation

@jfcrenshaw
Copy link
Copy Markdown
Contributor

No description provided.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jfcrenshaw jfcrenshaw force-pushed the tickets/SITCOM-1149_20241027 branch from fd0151d to 4fb7992 Compare October 28, 2024 21:58
@@ -0,0 +1,344 @@
{
Copy link
Copy Markdown
Contributor

@suberlak suberlak Oct 28, 2024

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

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Contributor

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).

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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

@lsst-so lsst-so deleted a comment from jfcrenshaw Oct 28, 2024
@jfcrenshaw
Copy link
Copy Markdown
Contributor Author

@jfcrenshaw
Copy link
Copy Markdown
Contributor Author

@@ -0,0 +1,382 @@
{
Copy link
Copy Markdown
Contributor

@suberlak suberlak Oct 29, 2024

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

3 participants