Skip to content

Dataset-saving infrastructure phase 1#688

Open
SolarDrew wants to merge 29 commits intoDKISTDC:mainfrom
SolarDrew:filesaving
Open

Dataset-saving infrastructure phase 1#688
SolarDrew wants to merge 29 commits intoDKISTDC:mainfrom
SolarDrew:filesaving

Conversation

@SolarDrew
Copy link
Contributor

Closes #671

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 9, 2026

Merging this PR will not alter performance

✅ 14 untouched benchmarks


Comparing SolarDrew:filesaving (44b5668) with main (36d88bd)1

Open in CodSpeed

Footnotes

  1. No successful run was found on main (f0f191e) during the generation of this report, so 36d88bd was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@SolarDrew
Copy link
Contributor Author

I'm sure there are other issues remaining but the bulk of the current error are due to slicing bugs in Inversion, which should be solved by #690

@SolarDrew
Copy link
Contributor Author

I've merged #690 and #695 for now so that the tests are actually testing the changes in this PR. Once those are merged I'll rebase and tidy up the history.

@SolarDrew
Copy link
Contributor Author

pre-commit.ci autofix

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

(Blocking because of git dependency)

"asdf-astropy>=0.5.0", # Required by gwcs 0.24
"asdf-coordinates-schemas>=0.3.0", # required by wcs-schemas 0.4
"asdf>=4.0.0", # Required by gwcs 0.24
"asdf-astropy@git+https://github.com/Cadair/asdf-astropy@highlevelwcswrapper", # Required for dataset.save()
Copy link
Member

Choose a reason for hiding this comment

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

This needs changing before merging.

Dataset loading and saving routines.
"""

def save(self, asdf_path=None, basepath=None, overwrite=False):
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if 3 .save() methods is better than dkist.io.save(anything_here)?

Dataset loading and saving routines.
"""

def save(self, asdf_path=None, basepath=None, overwrite=False):
Copy link
Member

Choose a reason for hiding this comment

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

I feel like defaulting to "put new asdf where old one is" is a curious choice? why don't we make asdf_path required? then we can also drop the base_path kwarg?

Co-authored-by: Stuart Mumford <stuart@cadair.com>
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.

Phase 1 - save modified datasets without modified data

2 participants