-
Notifications
You must be signed in to change notification settings - Fork 15
create empty and create_empty_like #687
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #687 +/- ##
==========================================
+ Coverage 85.30% 87.46% +2.15%
==========================================
Files 46 91 +45
Lines 2219 5115 +2896
Branches 306 346 +40
==========================================
+ Hits 1893 4474 +2581
- Misses 281 553 +272
- Partials 45 88 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks Dmitry. We should make it less specific to SEGY. I propose the following design:
Additionally, the tutorial notebook needs to be updated |
|
To add some further context
|
|
@tasansal, @BrianMichell def create_empty_mdio( # noqa PLR0913
mdio_template: AbstractDatasetTemplate,
dimensions: list[Dimension],
output_path: UPath | Path | str,
create_headers: bool = False,
overwrite: bool = False,
) -> None:
"""A function that creates an empty MDIO v1 file with known dimensions.
Args:
mdio_template: The MDIO template to use to define the dataset structure.
dimensions: The dimensions of the MDIO file.
output_path: The universal path for the output MDIO v1 file.
create_headers: Whether to create a full set of SEG-Y v1.0 trace headers. Defaults to False.
overwrite: Whether to overwrite the output file if it already exists. Defaults to False.
Raises:
FileExistsError: If the output location already exists and overwrite is False.
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the current implementation strikes a happy medium between our discussions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
teapod should be fixed to teapot. I think I caught all instance but please double check.
It looks like there was some scope creep in this PR which started updating tests like the test_segy_roundtrip_teapot source file. If you could please remove these and open a separate PR to keep changes limited in scope and reviews managable.
| assert not garbage_file.exists(), "Garbage file should have been overwritten" | ||
| assert not garbage_dir.exists(), "Garbage directory should have been overwritten" | ||
|
|
||
| def test_populate_empty_dataset(self, mdio_with_headers: Path) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the purpose of this as a test. If anything a more detailed docs page in the form of a jupyter notebook would be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intended purpose of the test is ensure that the structure of the created empty dataset is sufficient to allow the user to perform all the dataset population steps he/she might fancy. If/as the API evolves, the test allows to immediately update the example code to reflect the API changes.
Since jupyter notebook are not executed with every PR submission, they might become out of sync with the API (as any documentation tends to do). A purely hypothetical example: if we decide to change FEET_PER_SECOND to FOOT_PER_SECOND, the test will be properly updated as a part of the corresponding PR, while the jupyter notebook will be updated only after the user complains that it can't run any longer.
Also, please note that jupyter is not installed as a part of the standard/extended TGSAI/mdio-python environment. So, formally and pure hypothetically, we should not expect that our users use jupyter notebooks. After all, we do not test if TGSAI/mdio-python is compatible with jupyter.
Please advise, if you agree with the argument above or if you still prefer the test to be removed.
In order to avoid code duplication, I oppted
Please advise, if I should make create_empty tests suite independent of test_segy_roundtrip_teapod.py by copying some of the roundtrip_teapod setup code to avoid the problems with the above. |
Add new API
Please review tests/integration/test_create_empty.py : test_populate_empty_dataset() to see if it could be used to address