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

Issues/165 #166

Merged
merged 17 commits into from
Jun 7, 2018
Merged

Issues/165 #166

merged 17 commits into from
Jun 7, 2018

Conversation

wmwv
Copy link
Contributor

@wmwv wmwv commented May 31, 2018

Example notebook for loading a merged tract+patch from an HDF5 file and checking the stellar color-color locus.

@wmwv wmwv requested a review from djperrefort May 31, 2018 16:07
}
},
"source": [
"# Looking at merged tract-patch catalogs in Run 1.1 leading up to DC2\n",

Choose a reason for hiding this comment

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

From the title alone I'm not entirely sure what this notebook is intended to convey. A brief description at the beginning that explains what will be demonstrated would be useful. This might include instructions on any file paths the user needs to set, or simply that the notebook is intended to be run on NERSC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

"plt.xlabel('g-r')\n",
"plt.ylabel('r-i')\n",
"\n",
"plot_stellar_locus('gmr', 'rmi')"

Choose a reason for hiding this comment

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

FileNotFoundError: File b'Davenport_2014_MNRAS_440_3430_table1.txt' does not exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what directory are you running the notebook?

Choose a reason for hiding this comment

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

Just my user directory. I did a little directory tree exploration for the file but didn't see it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the same directory as the notebook. Did you use the NERSC Jupyter-dev interface to run this?

Choose a reason for hiding this comment

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

When ssh-ing into cori and cloning the DC2_Repo repo this error does not occur. Previously had an issue with my ssh access and was using the Jupyter upload interface. I simply forgot to upload this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

},
"outputs": [],
"source": [
"def simple_stellar_locus_gmr_rmi(color='red', linestyle='--', linewidth=2.5):\n",

Choose a reason for hiding this comment

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

Docstring for simple_stellar_locus_gmr_rmi would be useful for clarity. I'm specifically unclear on where the numbers 1.4, 0.5, 1.5, -0.5, ... come from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

"outputs": [],
"source": [
"z = galaxies\n",
"plt.scatter(z['g_mag'] - z['r_mag'], z['g_mag'], marker='.')\n",

Choose a reason for hiding this comment

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

Missing axes labels

Copy link
Contributor Author

@wmwv wmwv Jun 7, 2018

Choose a reason for hiding this comment

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

👍

"cell_type": "markdown",
"metadata": {},
"source": [
"Clean this up a bit to only include stars with SNR > 3 in g and r"

Choose a reason for hiding this comment

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

You say you are going to implement a cut for SNR > 3, but then impliment 3 different cutoffs. This left me with some minor but unnecessary confusion. Explain first that you are going to use different SNR to create 3 different catagories: good snr, bright stars, and galaxies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

"* The catalogs supporting DC2 are in:\n",
"https://github.com/LSSTDESC/gcr-catalogs\n",
"\n",
"See 'DC2_Static_Coadd_Catalog_reader.ipynb' notebook by Yao-Yuan Mao\n",

Choose a reason for hiding this comment

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

Link to DC2_Static_Coadd_Catalog_reader.ipynb for easy navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next line is that link. Does it show up for you?

Choose a reason for hiding this comment

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

Ah yes, for some reason I had thought that was something else. Not the notebook's fault, just a personal mental skip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

@yymao yymao left a 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 to me.

On a side note, we now start to have more example notebooks that use different methods to access DC2 products. While this is totally fine, sometimes it is difficult to people who are not following to navigate and to find the best option for them. We should document this somewhere to make things organized.

@wmwv
Copy link
Contributor Author

wmwv commented Jun 7, 2018

@yymao Agreed. I hope that we can stop at four. 1) GCR 2) DB 3) Merged catalog 4) Butler-based
And I further hope that we can reduced things largely to 1/2 for most use cases and 4 when you really have to dig in. But 3 currently requires the least infrastructure.

@wmwv wmwv merged commit dfb879a into master Jun 7, 2018
@yymao yymao deleted the issues/165 branch May 14, 2020 17:17
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.

None yet

3 participants