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

change order of key mapping transform #993

Merged
merged 4 commits into from
Feb 4, 2025
Merged

change order of key mapping transform #993

merged 4 commits into from
Feb 4, 2025

Conversation

misko
Copy link
Collaborator

@misko misko commented Feb 4, 2025

Currently we perform key_mapping renames and then perform data_object transforms. However data object transforms have hard coded string values. This means that if we change the task and change the key_mapping the same hard coded data transforms will no longer work, i.e. key omat24_energy might not be present.
On the flip side, if we perform data transforms before key name remapping, this cleans up the transforms since most datasets have the keys energy and not datasetname_energy.

This PR changes the order so that we first perform data transforms, and then perform key renaming.

@misko misko added the enhancement New feature or request label Feb 4, 2025
@misko misko added the patch Patch version release label Feb 4, 2025
@misko misko requested a review from rayg1234 February 4, 2025 01:07
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/fairchem/core/datasets/oc22_lmdb_dataset.py 0.00% 4 Missing ⚠️
src/fairchem/core/datasets/ase_datasets.py 50.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/fairchem/core/datasets/lmdb_dataset.py 80.00% <100.00%> (+0.20%) ⬆️
src/fairchem/core/datasets/ase_datasets.py 87.09% <50.00%> (ø)
src/fairchem/core/datasets/oc22_lmdb_dataset.py 15.20% <0.00%> (-0.13%) ⬇️

@rayg1234
Copy link
Collaborator

rayg1234 commented Feb 4, 2025

Thanks for cleaning this up!

@misko misko enabled auto-merge February 4, 2025 18:28
@misko misko added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit 62928eb Feb 4, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request patch Patch version release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants