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

Iolin #221

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Iolin #221

wants to merge 3 commits into from

Conversation

i-olin
Copy link
Contributor

@i-olin i-olin commented May 17, 2024

Grammar and typo corrections.

@i-olin i-olin requested review from rnikutta and stephjuneau May 17, 2024 20:27
Copy link
Member

@rnikutta rnikutta left a comment

Choose a reason for hiding this comment

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

@i-olin Thank for the PR! I left a few comments and change requests in-line.

@@ -16,7 +16,7 @@
"outputs": [],
"source": [
"__author__ = 'Ragadeepika Pucha <[email protected]>, Stephanie Juneau <[email protected]>'\n",
"__version__ = '20230914' \n",
"__version__ = '20230914' #yyyymmdd\n",
Copy link
Member

Choose a reason for hiding this comment

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

Update the date to current.

@@ -1638,9 +1638,9 @@
],
"metadata": {
"kernelspec": {
"display_name": "DESI 23.1",
"display_name": "Python 3 (ipykernel)",
Copy link
Member

Choose a reason for hiding this comment

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

Re-run this NB on the DL NB server, so that it can use the DESI 23.x kernel (this is important, because specific kernels usually have specific S/W installed)/

@@ -16,7 +16,7 @@
"outputs": [],
"source": [
"__author__ = 'Ragadeepika Pucha <[email protected]>, Stephanie Juneau <[email protected]>'\n",
"__version__ = '20230824' \n",
"__version__ = '20230824' #yyyymmdd\n",
Copy link
Member

Choose a reason for hiding this comment

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

Update the version whenever you change and NB.

@@ -1480,7 +1480,7 @@
],
"metadata": {
"kernelspec": {
"display_name": "Python 3",
"display_name": "Python 3 (ipykernel)",
Copy link
Member

Choose a reason for hiding this comment

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

Re-run on DL's NB server to use the DL Python 3 kernel.

@@ -3342,7 +3342,7 @@
],
"metadata": {
"kernelspec": {
"display_name": "Python 3",
"display_name": "Python 3 (ipykernel)",
Copy link
Member

Choose a reason for hiding this comment

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

Re-run on DL's NB server

@@ -350,7 +350,7 @@
"\n",
"The many S-PLUS photometric bands can be used to produce color-color diagrams that are useful for measuring properties such as the metallicity or surface gravity of a star.\n",
"\n",
"(g-F515) vs. (g-r) is sensitive to surface gravity and can be used to separate dwarf stars from giant stars. The main locus of points is the dwarfs will the red giant live in the upper right. [See Majewski et al. (2000)](http://adsabs.harvard.edu/abs/2000AJ....120.2550M) for more details on this method."
"(g-F515) vs. (g-r) is sensitive to surface gravity and can be used to separate dwarf stars from giant stars. The main focus of points is the dwarfs, while the red giant live in the upper right. [See Majewski et al. (2000)](http://adsabs.harvard.edu/abs/2000AJ....120.2550M) for more details on this method."
Copy link
Member

Choose a reason for hiding this comment

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

'locus' was correct, please change back. The sentence had typos though, and I think it should be "The main locus of points is the dwarfs, while the red giants live in the upper right."

@@ -2277,7 +2277,7 @@
"ax1 = fig.add_subplot(121)\n",
"ax2 = fig.add_subplot(122)\n",
"\n",
"# color-magnitude diagram (Hess diagram)\n",
"# color-color diagram\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

"Hess diagram" could be put back with a link to the short wikipedia page: https://en.wikipedia.org/wiki/Hess_diagram

Copy link
Contributor

@stephjuneau stephjuneau left a comment

Choose a reason for hiding this comment

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

Great work, Isabella! I agree with Robert's comments and made one more regarding the Hess diagram.

@stephjuneau
Copy link
Contributor

After discussion, @i-olin will create a new branch instead. Once the new branch is complete accounting for the changes requested here, and the new PR is merged, we will close this one.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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