Skip to content

Conversation

@FFroehlich
Copy link
Member

@FFroehlich FFroehlich commented Dec 2, 2024

CI

  • turn testcase runner into proper pytest
  • add github workflow

neural nets

  • fix non-passing networks tests involving convolutional layers (004-008, 021, 022, 046 - 048, 050)

amici core

tests

@codecov
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

❌ Patch coverage is 89.81818% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.25%. Comparing base (65deb50) to head (fa95339).

Files with missing lines Patch % Lines
python/sdist/amici/jax/petab.py 82.85% 18 Missing ⚠️
python/sdist/amici/de_model.py 84.00% 8 Missing ⚠️
python/sdist/amici/jax/nn.py 97.61% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2614      +/-   ##
==========================================
+ Coverage   76.48%   77.25%   +0.76%     
==========================================
  Files         308      309       +1     
  Lines       19540    19804     +264     
  Branches     1499     1500       +1     
==========================================
+ Hits        14945    15299     +354     
+ Misses       4582     4492      -90     
  Partials       13       13              
Flag Coverage Δ
cpp 72.75% <24.00%> (-0.14%) ⬇️
cpp_python 37.80% <7.63%> (-0.47%) ⬇️
petab 38.56% <25.45%> (-0.31%) ⬇️
petab_sciml 13.87% <88.00%> (?)
python 69.58% <26.18%> (-0.04%) ⬇️
sbmlsuite-jax 33.18% <14.18%> (-0.78%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
python/sdist/amici/de_export.py 96.89% <100.00%> (+<0.01%) ⬆️
python/sdist/amici/jax/__init__.py 100.00% <ø> (ø)
python/sdist/amici/jax/jaxcodeprinter.py 90.32% <100.00%> (+1.43%) ⬆️
python/sdist/amici/jax/model.py 86.85% <100.00%> (+0.30%) ⬆️
python/sdist/amici/jax/ode_export.py 88.33% <100.00%> (+1.06%) ⬆️
python/sdist/amici/petab/parameter_mapping.py 85.37% <100.00%> (+0.64%) ⬆️
python/sdist/amici/petab/petab_import.py 100.00% <100.00%> (ø)
python/sdist/amici/petab/sbml_import.py 74.60% <ø> (ø)
python/sdist/amici/petab/util.py 86.66% <ø> (ø)
python/sdist/amici/sbml_import.py 93.93% <100.00%> (ø)
... and 3 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@socket-security
Copy link

socket-security bot commented Oct 15, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatedinterpax@​0.3.6 ⏵ 0.3.11100100100100100

View full report

@FFroehlich FFroehlich marked this pull request as ready for review October 28, 2025 15:30
@FFroehlich FFroehlich requested a review from a team as a code owner October 28, 2025 15:30
Copy link
Member

@dweindl dweindl left a comment

Choose a reason for hiding this comment

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

Thanks. Just some initial feedback, for now. Will try to finish later this week.

Documentation is a bit sparse. It would be great to have docstrings in all functions/classes/modules.

Please also add an item to CHANGELOG.md.

Copy link
Member

Choose a reason for hiding this comment

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

Please add to python_examples.rst

ignore:Conservation laws for non-constant species in models with RateRules are currently not supported and will be turned off.:UserWarning
ignore:Conservation laws for non-constant species in models with Species-AssignmentRules are currently not supported and will be turned off.:UserWarning
ignore:Conservation laws for non-constant species in combination with parameterized stoichiometric coefficients are not currently supported and will be turned off.:UserWarning
ignore:PEtab v2.0.0 mapping tables are only partially supported:UserWarning
Copy link
Member

Choose a reason for hiding this comment

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

Still relevant? I couldn't see that warning anywhere.

allow_reinit_fixpar_initcond: bool | None = True,
generate_sensitivity_code: bool | None = True,
model_name: str | None = "model",
hybridisation: dict | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Please document

Parses the hybridisation information and updates the model accordingly
:param hybridization:
hybridization information
Copy link
Member

Choose a reason for hiding this comment

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

This could be more informative 😅
Is this documented in petab-sciml? Add reference?

}
if len(outputs.keys()) != len(net["output_vars"]):
raise ValueError(
f"Could not find all output variables for neural network {net_id}"
Copy link
Member

Choose a reason for hiding this comment

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

Include the missing ones in the message?

Comment on lines +1294 to +1297
True
if p
in set(self._parameter_mappings[sc].map_sim_var.keys())
else False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
True
if p
in set(self._parameter_mappings[sc].map_sim_var.keys())
else False
p in set(self._parameter_mappings[sc].map_sim_var.keys())

}
for net_id, net_config in config["neural_nets"].items()
}
if not jax or petab_problem.model.type_id == MODEL_TYPE_PYSB:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not jax or petab_problem.model.type_id == MODEL_TYPE_PYSB:
if not jax or petab_problem.model.type_id != MODEL_TYPE_SBML:

More future-proof.

Comment on lines +307 to +310


# for backwards compatibility
import_model = import_model_sbml
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# for backwards compatibility
import_model = import_model_sbml

This has been removed in main, I guess it's an unintentional merge leftover.

self.sbml_doc.validateSBML
)()
_check_lib_sbml_errors(self.sbml_doc, self.show_sbml_warnings)
# _check_lib_sbml_errors(self.sbml_doc, self.show_sbml_warnings)
Copy link
Member

Choose a reason for hiding this comment

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

Why?

# the SBMLError log in the sbml document. Thus, it is sufficient to
# check the error log just once after all conversion/validation calls.
_check_lib_sbml_errors(self.sbml_doc, self.show_sbml_warnings)
# _check_lib_sbml_errors(self.sbml_doc, self.show_sbml_warnings)
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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.

4 participants