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

abstractrawdataset updated #202

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Conversation

allaffa
Copy link
Collaborator

@allaffa allaffa commented Nov 5, 2023

This PR addresses two issues:

  • self.__normalize_dataset() is safeguarded by a boolean variable and used only when the user specifies it in the .JSON file
  • fixes a bug that was hidden inside the AbstractRawDataset .
    update_predicted_values and update_atom_features are already included in the SimplePickleDataset class. Therefore, there is no need to used them now inside AbstractRawDataset

@allaffa allaffa added the bug Something isn't working label Nov 5, 2023
@allaffa allaffa requested a review from jychoi-hpc November 5, 2023 19:46
@allaffa allaffa self-assigned this Nov 5, 2023
@allaffa
Copy link
Collaborator Author

allaffa commented Nov 5, 2023

This PR aims at fixing a bug that was hidden inside the AbstractRawDataset as a result of a transition from the old data management and the new data management methodologies of HydraGNN.
update_predicted_values and update_atom_features removed from AbstractRawDataset class because these functions now are already included in the SimplePickleDataset class.

@allaffa allaffa requested a review from pzhanggit November 5, 2023 19:50
Copy link
Collaborator

@pzhanggit pzhanggit 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. One comment about the feature update lines is that if we should keep them here and remove the ones in SimplePickleDataset since the data is loaded here?

@allaffa
Copy link
Collaborator Author

allaffa commented Nov 6, 2023

Looks good to me. One comment about the feature update lines is that if we should keep them here and remove the ones in SimplePickleDataset since the data is loaded here?

@pzhanggit that's a good point. AbstractRawDataset is meant to be an "intermediate" layer between the AbstractBaseDataset class and any fully specified dataset class.
Therefore, for some specific datasets, as we already do for some examples such as for the ORNL_AISD-Ex dataset, we inherit directly from AbstractBaseDataset.
SimplePickleDataset or AdiosDataset are always used no matter what, because we will always need to convert the data from raw format into pickle or adios.

Therefore, to me it seems like keeping the feature update lines into SimplePickleDataset and AdiosDataset is more robust.

@allaffa allaffa marked this pull request as draft November 6, 2023 16:42
@allaffa
Copy link
Collaborator Author

allaffa commented Nov 6, 2023

@pzhanggit @jychoi-hpc
I temporarily blocked this PR by converting it into draft.
I looked again at the implementation of AdiosDataset.
I want to talk with Jong. I think that what I did is very practical for the SimplePickleDataset, but not so sure for AdiosDataset

@allaffa allaffa marked this pull request as ready for review November 9, 2023 21:32
@allaffa
Copy link
Collaborator Author

allaffa commented Nov 9, 2023

@jychoi-hpc @pzhanggit
I gave another thought to this PR.
Many examples not currently included in the main branch also use update_predicted_values and update_atom_features . Because of this, the most practical way to ensure that these two capabilities are always available consists maintaining them inside the SimplePickleDataset class.

@allaffa allaffa merged commit a86efac into ORNL:main Nov 9, 2023
2 checks passed
@allaffa allaffa deleted the updatse_abstractrawdataset branch November 9, 2023 21:53
RylieWeaver pushed a commit to RylieWeaver/HydraGNN that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants