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

Field guide section on YSOs #35

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

Conversation

AshishMahabal
Copy link
Collaborator

Basic text and examples from Lynne. Starting this so that we can see what is missing and add that.

@dmitryduev
Copy link
Collaborator

Thanks @AshishMahabal. Please pull in the latest main and fix the linting errors.

@AshishMahabal
Copy link
Collaborator Author

Not sure what is failing. Lint checks only changed files.
I do get warnings, but no errors during build locally.

@dmitryduev
Copy link
Collaborator

You got trailing whitespaces in doc/field_guide__yso.md. Could you try re-installing the pre-commit hook? (And also pulling in the latest main)?

@dmitryduev dmitryduev changed the title adding the yso entry Field guide section on YSOs May 17, 2021
Copy link
Collaborator

@dmitryduev dmitryduev left a comment

Choose a reason for hiding this comment

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

Thanks @AshishMahabal. I have a few minor comments.

Comment on lines +89 to +100
cv_U_Gem:
coordinates: [18.884181, 37.626534]
title: Cataclysmic variable (U Gem dwarf nova)
cv_Z_Cam:
coordinates: [269.093260, 2.967803]
title: Cataclysmic variable (Z Cam dwarf nova)
cv_SU_UMa:
coordinates: [170.883476, 43.288214]
title: Cataclysmic variable (SU UMa/WZ Sge dwarf nova)
cv_Novalike:
coordinates: [322.961689, 49.233813]
title: Cataclysmic variable (Novalike)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already on main, looks like you need to rebase this branch off it or just pull it in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would the commands be?

@@ -0,0 +1,101 @@
ra,dec
Copy link
Collaborator

Choose a reason for hiding this comment

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

The RA/Dec plot will be like a single blob. Do we want to get more generic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will add objects from other parts later.

Comment on lines +9 to +11



Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the linting works only on modified files, I did this file by hand (when I had the malloc errors I did some python plus venv switching, and for some reason this file didn't throw lint errors on my terminal, but did when committed). The blank lines seemed to have escaped.

![RA/Dec diagram of YSO](data/radec__yso.png)

### References and further reading:
-
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need references.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, Lynne is to provide some.

@dmitryduev
Copy link
Collaborator

@AshishMahabal ping!

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

2 participants