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.

Sign up for GitHub

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

Remove units from MOS pipeline. #1441

Closed
wants to merge 48 commits into from
Closed

Remove units from MOS pipeline. #1441

wants to merge 48 commits into from

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Oct 7, 2024 •
edited
Loading

Resolves RCAL-911

Closes #

This PR addresses ...

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.patch_match.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Oct 7, 2024
mairanteodoro and others added 24 commits October 7, 2024 12:18
@mairanteodoro
First refactoring of TweakRegStep to use stcal.
953961a
@mairanteodoro
Code cleanup/refactoring.
89bd44e
@mairanteodoro
Replace local method with stcal's.
c033702
@mairanteodoro
Further code refactoring.
f015feb
@pre-commit-ci @mairanteodoro
[pre-commit.ci] auto fixes from pre-commit.com hooks
a99761c 
for more information, see https://pre-commit.ci
@mairanteodoro
Utilize stcal's get_catalog method.
79b667c
@pre-commit-ci @mairanteodoro
[pre-commit.ci] auto fixes from pre-commit.com hooks
42fef2c 
for more information, see https://pre-commit.ci
@mairanteodoro
Add clip_accum=True to call to alignment methods.
8991f9f
@mairanteodoro
Point stcal installation to main branch.
76eb339
@mairanteodoro
Revert some refactoring.
7091433
@mairanteodoro
Refactor handling of invalid exposure types.
2313559
@pre-commit-ci @mairanteodoro
[pre-commit.ci] auto fixes from pre-commit.com hooks
f316920 
for more information, see https://pre-commit.ci
@mairanteodoro
Address comments 2.
9d48114
@pre-commit-ci @mairanteodoro
[pre-commit.ci] auto fixes from pre-commit.com hooks
a97482a 
for more information, see https://pre-commit.ci
@mairanteodoro
Style refactoring.
c365437
@braingram @mairanteodoro
remove default values from docs
4876457
@braingram @mairanteodoro
add changelog
1b6729a
@braingram @mairanteodoro
fix warning
be13903
@zacharyburnett @mairanteodoro
[CI] rename changelog check job to be more explicit on its purpose (s…
89e7148 
…pacetelescope#1422)
@zacharyburnett @mairanteodoro
update pull request checklist (spacetelescope#1336)
7ea6e0a
@mairanteodoro @pre-commit-ci
@zacharyburnett @braingram
RCAL-895: allow updating source catalog with tweaked WCS when running…
2f96fba 
… ELP (spacetelescope#1373)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Zach Burnett <[email protected]>
Co-authored-by: Brett <[email protected]>
@braingram @mairanteodoro
fix artifactory_repos for pytest 8
91e9794
@braingram @mairanteodoro
remove MultilineLogger
6fb921c
@braingram @mairanteodoro
remove unused variables
cc62ed2
braingram and others added 18 commits October 7, 2024 12:19
@github-actions github-actions bot added documentation Improvements or additions to documentation testing associations automation labels Oct 7, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.12%. Comparing base (2383105) to head (4abdef1).

❗ There is a different number of reports uploaded between BASE (2383105) and HEAD (4abdef1). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (2383105) HEAD (4abdef1)
2 0
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #1441       +/-   ##
===========================================
- Coverage   78.38%   62.12%   -16.27%     
===========================================
  Files         118       76       -42     
  Lines        7680     3200     -4480     
===========================================
- Hits         6020     1988     -4032     
+ Misses       1660     1212      -448
Flag Coverage Δ *Carryforward flag
nightly 62.12% <ø> (ø) Carriedforward from 2383105

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stscijgbot-rstdms stscijgbot-rstdms mentioned this pull request Oct 7, 2024
Closed
schlafly
schlafly reviewed Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

This looks good to me for now, but we will want to hold off merging until we remove the need for the RegtestFileModifier. I also left one comment about a ".value" that confused me.

romancal/flux/flux_step.py
[log.error(line) for line in message.splitlines()]
raise ValueError(message)
log.info(message)
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just delete this stanza. Do we special case the "already complete" step anywhere else?

romancal/source_catalog/source_catalog.py
@@ -162,7 +160,7 @@ def pixel_area(self):
pixel_area = self.model.meta.photometry.pixelarea_steradians
if pixel_area < 0:
pixel_area = (self._pixel_scale**2).to(u.sr)
return pixel_area
return pixel_area.value
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 fine, though I'm a little confused since I thought you removed the units from model.meta.photometry.

@mairanteodoro
Copy link
Collaborator Author

Closing this PR, since the work from here was merged into PR 1445.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlafly schlafly schlafly left review comments

Assignees
No one assigned
Labels
associations automation dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation no-changelog-entry-needed pipeline regression_testing testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mairanteodoro @schlafly @braingram @zacharyburnett