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

Jump to bottom

feat: DAMD-146 (add proposal_id and ob_code to pfsDesign/pfsConfig fi… #118

Merged
merged 2 commits into from Sep 14, 2023
Merged

feat: DAMD-146 (add proposal_id and ob_code to pfsDesign/pfsConfig fi… #118

merged 2 commits into from Sep 14, 2023

Conversation

monodera
Copy link
Contributor

…les)

  • proposalId and obCode are added.
  • datamodel.txt and test_PfsConfig.py are modified accordingly.

PaulPrice
PaulPrice requested changes Jul 17, 2023
View reviewed changes
Copy link
Member

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

test_PfsFluxReference.py fails due to this change, and I'm sure there are similar tests in drp_stella that fail too.

datamodel.txt Show resolved Hide resolved
python/pfs/datamodel/pfsConfig.py Outdated
proposalId : `numpy.chararray`
Proposal ID of each fiber (e.g, S23A-001QN).
obCode : `numpy.chararray`
OB code of each fiber.
Copy link
Member

Choose a reason for hiding this comment

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

Spell out OB: "Observing block".

@PaulPrice
Copy link
Member

Please squash related changes, and use git rebase instead of merging master into the ticket branch.

@monodera
Copy link
Contributor Author

monodera commented Sep 6, 2023

I think that I have squashed everything suggested by git rebase in this and the other related repositories. I've also run the integration test locally and succeeded.

@PaulPrice
Copy link
Member

The current state is a bit of a mess, with multiple commits with the same title.

@monodera
Copy link
Contributor Author

monodera commented Sep 6, 2023

What can I do from now to clean up thing?

@PaulPrice
Copy link
Member

If you're at NAOJ, perhaps @SogoMineo is able to help guide you through the git labyrinth?

@monodera
Copy link
Contributor Author

monodera commented Sep 6, 2023

we are in different branches, but I'll ask him.

@monodera
Add descriptions for proposalId and obCode
751a573 
- Add descriptions proposalId and obCode to datamodel.txt
@monodera
Add proposalId and obCode to pfsDesign/pfsConfig
b73f697 
- Operation-related keys `proposalId` and `obCode` are added to
  pfsDesign and pfsConfig files.

- Modify relevant tests to accommodate `proposalId` and `obCode` keys.
@monodera
Copy link
Contributor Author

monodera commented Sep 7, 2023

With the great help of @SogoMineo, I restarted editing from the current master branch and force-pushed the commits as shown in the tree.

A few places differ from the older commits, but these lines are duplicates from my wrong edit in the previous rebase. You can expand the code and find the identical lines a few lines below the removed ones.

PaulPrice
PaulPrice approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

Very nice. Thank you, @SogoMineo! And thank you @monodera for bearing with me.

@monodera
Copy link
Contributor Author

Can I merge to the master or you will merge, @PaulPrice ?

@monodera
Copy link
Contributor Author

@SogoMineo told me that I can push the green "merge pull request" button as encouraged in the documentation, so I'll do it.

@monodera
Copy link
Contributor Author

@PaulPrice When merging a PR, which method should I use from the menu, "Create a merge commit", "Squash and merge", and "Rebase and merge"?

Also, could you please take a look at these related branches in drp_stella and pfs_utils?
Subaru-PFS/drp_stella#330
Subaru-PFS/pfs_utils#65

@PaulPrice
Copy link
Member

Please use the "Create a merge commit" method.
The others look fine too.
You've run these through the integration test, right?

@monodera
Copy link
Contributor Author

Yes, it finished with the "Done with Gen3." message.

@monodera monodera merged commit bff215b into master Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulPrice PaulPrice PaulPrice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@monodera @PaulPrice