Skip to content

Fix legacy attributes #328

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 14, 2025
Merged

Fix legacy attributes #328

merged 1 commit into from
Aug 14, 2025

Conversation

GianlucaFicarelli
Copy link
Collaborator

@GianlucaFicarelli GianlucaFicarelli commented Aug 12, 2025 •
edited
Loading

  • Replace ScientificArtifactFilterBase.contact_id (uuid) with contact_email (str)
  • Remove ScientificArtifactBase.atlas_id because it doesn't exist in the db model

Note: ScientificArtifact.atlas_id should be removed from entitysdk as well

@GianlucaFicarelli
Fix legacy attributes
5cda290 
- Replace ScientificArtifactFilterBase.contact_id (uuid) with contact_email (str)
- Remove ScientificArtifactBase.atlas_id because it doesn't exist in the db model
@GianlucaFicarelli GianlucaFicarelli self-assigned this Aug 12, 2025
@GianlucaFicarelli
Copy link
Collaborator Author

Note that ScientificArtifactBase.atlas_id was added to the pydantic schema in #212 but I cannot find a reference of ScientificArtifact.atlas_id in model.py, so I think it's safe to be removed from the pydantic schema as well.

@GianlucaFicarelli
Copy link
Collaborator Author

For confirmation or discussion: is it correct that the db model of ScientificArtifact doesn't contain atlas_id?
Or at the contrary is it needed for all the classes inheriting from scientific artifact?
Note that atlas_id has been explicitly added to the circuit class, but not to scientific_artifact.

@darshanmandge @AurelienJaquier

mgeplf
mgeplf approved these changes Aug 12, 2025
View reviewed changes
Copy link
Collaborator

@mgeplf mgeplf left a comment

Choose a reason for hiding this comment

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

Code is fine; consider this a LGTM for that side - whether atlas_id needs to exist is something I don't know.

@danielaegassan
Copy link

@GianlucaFicarelli I have asked about this in the scientific meeting. I has been agreed that we can remove atlas_id from scientific artifact and instead add it explicitly in each artifact this way we can also control better when it's mandatory, optional or not present.

@GianlucaFicarelli
Copy link
Collaborator Author

@GianlucaFicarelli I have asked about this in the scientific meeting. I has been agreed that we can remove atlas_id from scientific artifact and instead add it explicitly in each artifact this way we can also control better when it's mandatory, optional or not present.

Thanks, then I'm going to merge this PR as is.

@GianlucaFicarelli GianlucaFicarelli merged commit af203de into main Aug 14, 2025
1 check passed
@GianlucaFicarelli GianlucaFicarelli deleted the fix-legacy-attributes branch August 14, 2025 09:42
@GianlucaFicarelli GianlucaFicarelli mentioned this pull request Aug 14, 2025
Merged
GianlucaFicarelli added a commit that referenced this pull request Aug 18, 2025
@GianlucaFicarelli
Merge remote-tracking branch 'origin/main' into scientific-ion-channel
991a10e 
* origin/main:
  Add filter by authorized_public and authorized_project_id (#335)
  Fix legacy attributes (#328)
  Add circuit hierarchy endpoint (#320)
  Remove contribution from publication (#331)
GianlucaFicarelli added a commit that referenced this pull request Aug 20, 2025
@GianlucaFicarelli
Merge remote-tracking branch 'origin/main' into external_url
82f95f1 
* origin/main:
  Add Calibration & Validation activities (#307)
  Update uv and relax uv pinning in Dockerfile (#336)
  Add filter by authorized_public and authorized_project_id (#335)
  Fix legacy attributes (#328)
GianlucaFicarelli added a commit that referenced this pull request Aug 21, 2025
@GianlucaFicarelli
Merge remote-tracking branch 'origin/main' into em_reconstruction_v2
0319c4f 
* origin/main:
  IonChannelModel inheriting from ScientificArtifact (#300)
  Add external data source and url (#327)
  Add Calibration & Validation activities (#307)
  Update uv and relax uv pinning in Dockerfile (#336)
  Add filter by authorized_public and authorized_project_id (#335)
  Fix legacy attributes (#328)
  Add circuit hierarchy endpoint (#320)
  Remove contribution from publication (#331)
GianlucaFicarelli added a commit that referenced this pull request Aug 28, 2025
@GianlucaFicarelli
Merge remote-tracking branch 'origin/main' into draft-into-morphology…
a58a81e 
…-model

* origin/main:
  Add the ability to filter by name to IonChannelModel (#338)
  IonChannelModel inheriting from ScientificArtifact (#300)
  Add external data source and url (#327)
  Add Calibration & Validation activities (#307)
  Update uv and relax uv pinning in Dockerfile (#336)
  Add filter by authorized_public and authorized_project_id (#335)
  Fix legacy attributes (#328)
  Add circuit hierarchy endpoint (#320)
  Remove contribution from publication (#331)
  Add timezone to scientific_artifact.experiment_date (#326)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgeplf mgeplf mgeplf approved these changes

@darshanmandge darshanmandge Awaiting requested review from darshanmandge

@AurelienJaquier AurelienJaquier Awaiting requested review from AurelienJaquier

Assignees

@GianlucaFicarelli GianlucaFicarelli

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GianlucaFicarelli @danielaegassan @mgeplf