Skip to content

general dataloader#31

Merged
bekaiser-LANL merged 15 commits intolanl:mainfrom
soumide1102:general_dataloader_yoke
May 2, 2025
Merged

general dataloader#31
bekaiser-LANL merged 15 commits intolanl:mainfrom
soumide1102:general_dataloader_yoke

Conversation

@soumide1102
Copy link
Copy Markdown
Contributor

@hickmank this should be ready for your review.

Don't know if Bryan can look at this PR as I am not being able to add him as a reviewer. Maybe he can see this once he's added to lanl github?

@soumide1102 soumide1102 requested a review from hickmank April 1, 2025 23:00
@soumide1102 soumide1102 self-assigned this Apr 1, 2025
@bekaiser-LANL
Copy link
Copy Markdown
Contributor

Hi @soumide1102 - we need to change "return start_img, end_img, Dt, self.channel_map" to "return start_img, self.channel_map, end_img, self.channel_map, Dt" to align with what @hickmank wants the method to return, i.e., it should pass the t=t_0 array, the t=t_0 map, the t=t_1 array, the t=t_1 map, and Dt.

@hickmank hickmank requested a review from bekaiser-LANL April 2, 2025 14:23
Copy link
Copy Markdown
Contributor

@bekaiser-LANL bekaiser-LANL left a comment

Choose a reason for hiding this comment

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

need to change "return start_img, end_img, Dt, self.channel_map" to "return start_img, self.channel_map, end_img, self.channel_map, Dt" to align with what hickmank wants the method to return, i.e., it should pass the t=t_0 array, the t=t_0 map, the t=t_1 array, the t=t_1 map, and Dt. Also, the sequential_Dataset does not accommodate for changing channel maps as the sequence progresses. I think that's ok, we can do that later (if we ever need to)

Copy link
Copy Markdown
Collaborator

@hickmank hickmank left a comment

Choose a reason for hiding this comment

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

I don't think this will pass our linting right now. If you need help running that locally let me know.

  1. The positions need to be passed through meshgrid so they are 2D arrays like the rest of the channels.
  2. You need a 2D-array defining the detonator positions as a channel
  3. If there are two parts with the same material those fields need to be added together into a single channel. Can you point me to this logic in this PR.
  4. You need tests written for these new classes and functions.

Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
Comment thread src/yoke/datasets/load_npz_dataset.py Outdated
@galegozi
Copy link
Copy Markdown
Contributor

galegozi commented Apr 7, 2025

Hi @soumide1102 ,
I think your PR looks good.
I left a couple of comments regarding the coding style (not linting-related). These are just a couple of small things I thought that if they were fixed would make this PR better, thanks!

@hickmank
Copy link
Copy Markdown
Collaborator

@bekaiser-LANL @soumide1102 Looks like the dataset file needs to be linted. But, this PR is also changing a LOT of files. I think something is strange about how your branches are being merged. Can you take a look at it?

@soumide1102
Copy link
Copy Markdown
Contributor Author

soumide1102 commented Apr 14, 2025

yeah, I'm trying to get rid of the extra commits. Looks like Bryan's copy of this branch picked up these already merged commits somehow.

@soumide1102 soumide1102 force-pushed the general_dataloader_yoke branch from 8e442ef to 91bf3e4 Compare April 14, 2025 20:57
@hickmank
Copy link
Copy Markdown
Collaborator

@soumide1102 @bekaiser-LANL This looks great. Functionally, very good. You just need to get it through the linter and write tests.

soumide1102 and others added 7 commits April 17, 2025 11:23
@soumide1102 soumide1102 force-pushed the general_dataloader_yoke branch from 1c8e8cc to 8ef99c6 Compare April 17, 2025 17:24
@hickmank
Copy link
Copy Markdown
Collaborator

hickmank commented May 1, 2025

@soumide1102 @bekaiser-LANL let's get this merged in. Let me know what you need help with.

@soumide1102
Copy link
Copy Markdown
Contributor Author

@hickmank I'll push the tests in a few minutes. Just fixing the last linting complaints for the tests.

@soumide1102
Copy link
Copy Markdown
Contributor Author

@hickmank @bekaiser-LANL I've pushed tests for the npz dataloader. If you're fine with the additions, we can merge this.

@hickmank hickmank self-requested a review May 1, 2025 17:24
Copy link
Copy Markdown
Collaborator

@hickmank hickmank left a comment

Choose a reason for hiding this comment

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

Looks good to me! Yay!

@hickmank
Copy link
Copy Markdown
Collaborator

hickmank commented May 1, 2025

@bekaiser-LANL you need to re-review to unblock the merge. Assuming you're okay with the present state being merged.

@hickmank
Copy link
Copy Markdown
Collaborator

hickmank commented May 2, 2025

@bekaiser-LANL re-review please so we can merge this!

@bekaiser-LANL bekaiser-LANL merged commit 812fe38 into lanl:main May 2, 2025
3 checks passed
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.

4 participants