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.

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

Updates for CADET-Python-Simulator #147

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Updates for CADET-Python-Simulator #147

merged 4 commits into from
Dec 6, 2024

Conversation

schmoelder
Copy link
Contributor

This PR contains some modifications of CADET-Process to run the CADET-Python-Simulator.

Since the CADET-Python-Simulator is still very much experimental, let's see if / when we merge these changes to CADET-Process.

@schmoelder schmoelder marked this pull request as draft June 19, 2024 13:24
@schmoelder schmoelder force-pushed the feature/updates_for_simulator branch from e1b7a9a to 4f333b7 Compare August 26, 2024 09:36
@schmoelder schmoelder force-pushed the dev branch 11 times, most recently from 35e0c67 to d97cf31 Compare December 4, 2024 16:47
@schmoelder schmoelder requested a review from daklauss December 5, 2024 10:21
@schmoelder schmoelder marked this pull request as ready for review December 5, 2024 10:21
@schmoelder
Copy link
Contributor Author

@daklauss I know that we currently don't need these changes anymore for CADET-Python-Simulator, but I think, they are still don't hurt to have.

Could you please review / test?

@schmoelder schmoelder added this to the v0.10.0 milestone Dec 5, 2024
daklauss
daklauss requested changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@daklauss daklauss left a comment

Choose a reason for hiding this comment

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

See comments. Otherwise a test of densities for coverage like for the molecular_weight as in
https://github.com/fau-advanced-separations/CADET-Process/blob/dev/tests/test_components.py
would be a good thing.

CADETProcess/processModel/componentSystem.py Show resolved Hide resolved
CADETProcess/dynamicEvents/section.py Outdated Show resolved Hide resolved
@daklauss daklauss force-pushed the feature/updates_for_simulator branch from 4f333b7 to c3562a6 Compare December 5, 2024 23:29
@daklauss daklauss force-pushed the feature/updates_for_simulator branch from c3562a6 to f7d29bd Compare December 5, 2024 23:58
@daklauss daklauss self-requested a review December 6, 2024 00:14
@daklauss
Copy link
Collaborator

daklauss commented Dec 6, 2024 •
edited
Loading

Oh ... i guess i shouldn't have amended my changes directly to your add densities commit until it is ready to merge .... i think it is ready to merge but mb you can look over it @schmoelder. I added description and inits for densities and added a test for densities.

@schmoelder
Copy link
Contributor Author

schmoelder commented Dec 6, 2024 •
edited
Loading

Thanks Daniel, (almost) looks good to me, just some minor details.

Also, without wanting to overload this PR, what do you think about adding type annotations to entire module? Btw, this is something where tools like Copilot / ChatGPT can really do the bulk of the work.

@daklauss daklauss force-pushed the feature/updates_for_simulator branch 2 times, most recently from 9f41139 to 22e147c Compare December 6, 2024 09:11
@daklauss
Copy link
Collaborator

daklauss commented Dec 6, 2024

Added type annotations with gpt. Fixed some minor ai stuf. Review needed @schmoelder. Ready to merge afterwards.

@daklauss daklauss force-pushed the feature/updates_for_simulator branch from d6c4ebd to 96f6499 Compare December 6, 2024 10:22
@daklauss daklauss force-pushed the feature/updates_for_simulator branch from 2b4b3de to 8fec853 Compare December 6, 2024 11:16
@daklauss
Add type annotations
2cdd18e 
Fix Annotations

Fix annotations and Remove unused annotations
@daklauss daklauss force-pushed the feature/updates_for_simulator branch from 8fec853 to 2cdd18e Compare December 6, 2024 11:27
@schmoelder schmoelder merged commit 7039ef0 into dev Dec 6, 2024
4 checks passed
@schmoelder schmoelder deleted the feature/updates_for_simulator branch December 6, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daklauss daklauss daklauss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@schmoelder @daklauss