-
Notifications
You must be signed in to change notification settings - Fork 49
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
Major updates #258
Major updates #258
Conversation
…g mydb in one case. - No more need for static data files, deleted all. - Redone much of query and bookkeeping logic to simplify workflows. Shorted the NB a lot. - Renamed the ipynb file. - Rewrote Goals and Intro. - Cleaned up Imports. - Updated figures and figure-making functions. - Added docstrings to many functions. - Added/rewrote/streamlined story-telling text across the NB. - Synced TOCs with NB headings.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
I just added to this PR: nbid.csv with this NB ID added (0068), and updated keywords.txt. |
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.
Just a few corrections and non-critical suggestions on a very fast check:
-
In "Summary", change "currently under way on the CTIO Blanco 4m telescope" with "currently under way on the NOIRLab/Kitt Peak Mayall 4m telescope".
-
Adding the acronym after each survey would be helpful (DECaLS, BASS, MzLS) for people to recognize or look them up more quickly.
-
I would say, drop the ", including" from "...from the overview paper, including:" since that seems to imply you will next mention a few of the plots made in the notebook, not all of them, which is the case after the colon. Or, replace it with "which are".
-
Change "Fig 1: Query the catalogs..." with "Fig 2: Query the catalogs...".
-
In the first figure (Fig. 2) I wonder is there's a way to get a white background, much more desirable than dark grey. I noticed this is introduced by the masked_less_equal function on values=0 (and I didn't find a parameter to change it to white or none) which was done in order to eventually plot with a logarithmic scale, but we are not doing that here. Do you think that's necessary? Maybe commenting or removing that m_masked definition and plotting m instead of m_masked?
-
Typo: change "query that positinally JOINs" with "query that positionally joins".
-
Maybe the timeout=600 is not necessary any longer when defining dfx, since it went down to few seconds?
-
In plotting Fig. 8: I still prefer the look with nbins = 300 instead of 100, but I guess it's fine either way.
-
Typo: In Fig 15. first cell, change "which contai information" with "which contain information"
(The way you shorten and made more efficient the process for Fig. 15 is remarkable. A lot to learn from it.) -
In plotting Fig. 15, I would change 'violet' with 'purple'. That violet looks like a faint pink, gets too low of a profile in the plot compared to the other lines.
-
In first block of Fig 16: I would replace those ';' with ':' in the first sentence.
-
Typo: same block, replace "Finallu" with "Finally".
-
Typo: same block, replace "selectred" with "selected".
Thanks @kareninysimba , working on those now.. |
@kareninysimba Implemented and pushed all changes, except: - In the first figure (Fig. 2) I wonder is there's a way to get a white background, much more desirable than dark grey. I noticed this is introduced by the masked_less_equal function on values=0 (and I didn't find a parameter to change it to white or none) which was done in order to eventually plot with a logarithmic scale, but we are not doing that here. Do you think that's necessary? Maybe commenting or removing that m_masked definition and plotting m instead of m_masked? --> I was trying something that works in matplotlib:
but alas it doesn't work in hp.projview yet. Removed the masked array part as per your suggestion. - Maybe the timeout=600 is not necessary any longer when defining dfx, since it went down to few seconds? --> I've been running the queries many many times over the past few days, and when the machines/services were busy, the runtime could grow quite a bit. Let's leave the timeout at 600 just in case. - In plotting Fig. 8: I still prefer the look with nbins = 300 instead of 100, but I guess it's fine either way. --> With 300, the relation gets very tight, and one doesn't see the smeared-out broader distribution. Decided to keep it at 100. - In plotting Fig. 15, I would change 'violet' with 'purple'. That violet looks like a faint pink, gets too low of a profile in the plot compared to the other lines. --> Purple looks so close to both red and blue. How about a compromise of "awesome orange"? |
Sounds good. I try it and awesome orange seems fine :) |
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 see the semicolons in the first paragraph of Fig. 16 section (last plot). I still think is better to replace them with colons, i.e. change
...EXP (exponential disk; spiral galaxies), or DEV (de Vaucouleurs profile; elliptical galaxies)
with
...EXP (exponential disk: spiral galaxies), or DEV (de Vaucouleurs profile: elliptical galaxies)
Semicolon is eventually to separate ideas/sentences more or less semi-related or unrelated
You can also use "i.e." to replace the ";"
Hi all! This notebook looks really good. I didn't get any errors running it, I verified that all the links work, and the code looks solid. I only found a few spelling/grammar mistakes. Here is my feedback for DESILegacySurveys.ipynb:
|
Thank you @bmerino95 , I implemented your comments. Please approve and I'll merge. |
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 notebook looks good, and I have verified that the changes have been made.
Thank you @bmerino95 , merging! |
🎉🕺🏻🎊🕺🏻🎈🕺🏻 |
The commit message:
@kareninysimba @jacquesalice @bmerino95 can you please give this a review? If you only have minor comments, we can merge and feature this in the AAS Newsletter. If you find major issues, we'd have to push for a later release. Thank you!