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

WIP dataclass to BaseModel migration #72

Closed
wants to merge 5 commits into from
Closed

Conversation

talonchandler
Copy link
Contributor

No description provided.

Comment on lines +6 to +10
global_settings:
- &LCA_DAC 'TS1_DAC01'
- &LCB_DAC 'TS1_DAC02'
- &MCL_DAC 'TS1_DAC06'
- &AP_GALVO_DAC 'TS2_DAC03'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These needed to move to their own global_settings or else they would appear as extra parameters.

Comment on lines +58 to +63
# def test_example_settings(example_settings):
# s = AcquisitionSettings(**example_settings)

# # Check values match example file
# assert s.ls_microscope_settings.z_sequencing_settings[0].device_name == "TS2_DAC03"
# assert s.time_settings.num_timepoints == 10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't easily get these tests to pass when I migrated to BaseModel.

Comment on lines +96 to +104
class AcquisitionSettings(NoExtrasModel):
global_settings: Optional[List[str]] = None
time_settings: TimeSettings
lf_channel_settings: ChannelSettings
lf_slice_settings: SliceSettings
lf_microscope_settings: MicroscopeSettings
ls_channel_settings: ChannelSettings
ls_slice_settings: SliceSettings
ls_microscope_settings: MicroscopeSettings
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This top-level class was necessary to check for top-level extra parameters.

Comment on lines +50 to +51
num_channels: int = 0 #
acquisition_rate: Optional[float] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the type of logic I'm scared of in this migration. field isn't supported by BaseModel, so I need to change it, but I'm not sure that I've replicated the correct behavior.

@talonchandler
Copy link
Contributor Author

This was a failed attempt.

I learned the hard way that I should treat dataclass objects as completely different from BaseModel objects because they support different features and it's not easy to migrate between them.

@ieivanov ieivanov deleted the improve-pydantic branch August 19, 2023 00:40
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.

1 participant