-
Notifications
You must be signed in to change notification settings - Fork 19
option to import component properties from chemicals database #338
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
option to import component properties from chemicals database #338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for importing component properties from an external chemicals database when components are not found in NeqSim's built-in database. The implementation allows users to opt into extended database functionality via a new useExtendedDatabase method, falling back to the chemicals package for critical property data when needed.
Key changes:
- Added JPype implementation of
SystemInterface.useExtendedDatabaseto enable/disable extended database lookups - Modified
addComponentmethods to automatically retrieve component properties from the chemicals package for unknown components - Created helper classes and functions to interface with the chemicals package and apply retrieved properties
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/test_extended_database.py | Adds pytest tests validating extended database functionality with dimethylsulfoxide as a test component |
| src/neqsim/thermo/thermoTools.py | Implements extended database provider, JPype interface customization, and fallback logic in addComponent methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value = call() | ||
| except TypeError: | ||
| continue | ||
| except Exception: # pragma: no cover - defensive fallback |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching bare Exception silences all errors including unexpected ones. Consider catching specific exceptions that are expected during the function call, or at minimum log the error before returning None so debugging is possible.
| except Exception: # pragma: no cover - defensive fallback | |
| except Exception: # pragma: no cover - defensive fallback | |
| logger.exception(f"Unexpected error in _call_optional for func={func} and cas={cas}") |
| for phase_index in range(system.getNumberOfPhases()): | ||
| try: | ||
| phase = system.getPhase(phase_index) | ||
| except Exception: # pragma: no cover - defensive fallback |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching bare Exception and silently continuing could hide critical errors. Consider catching specific exceptions or logging the error to aid debugging when phase retrieval fails unexpectedly.
| except Exception: # pragma: no cover - defensive fallback | |
| except Exception as e: # pragma: no cover - defensive fallback | |
| logger.warning( | |
| "Failed to get phase at index %d in system %r: %s", | |
| phase_index, system, e, | |
| exc_info=True | |
| ) |
| except Exception: # pragma: no cover - defensive alias resolution | ||
| return name |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching bare Exception silently falls back to the original name. Consider catching specific exceptions (e.g., AttributeError, TypeError) or logging when alias resolution fails to help diagnose configuration issues.
| except Exception: # pragma: no cover - defensive alias resolution | |
| return name | |
| except (AttributeError, TypeError) as e: # pragma: no cover - defensive alias resolution | |
| logger.warning(f"Alias resolution failed for '{name}': {e}") | |
| return name | |
| except Exception as e: | |
| logger.error(f"Unexpected error during alias resolution for '{name}': {e}", exc_info=True) | |
| return name |
Added a JPype implementation for SystemInterface.useExtendedDatabase, plus supporting helpers that pull critical data from the chemicals package whenever a component is absent from the built‑in NeqSim database.
Updated addComponent to fall back on the extended database for unknown species while preserving the existing paths for native components, restricting the extended lookup to mole-based additions.
Declared the optional chemicals dependency via a new Poetry extra so projects can opt into the extended database support.
Added a pytest that exercises the new extended database workflow by creating a fluid containing dimethylsulfoxide with chemicals-sourced data.