SciML / SciMLBase.jl

The Base interface of the SciML ecosystem
https://docs.sciml.ai/SciMLBase/stable
MIT License
130 stars 96 forks source link

Remove unnecessary branch in `getindex()` #447

Closed staticfloat closed 1 year ago

staticfloat commented 1 year ago

This branch does not seem to be necessary, and also encodes the requirement that sys has a name property, which may not always be the case.

ChrisRackauckas commented 1 year ago

and also encodes the requirement that sys has a name property, which may not always be the case.

What sys doesn't have a name?

ChrisRackauckas commented 1 year ago

Which other branch handles the high level name space?

codecov[bot] commented 1 year ago

Codecov Report

Merging #447 (7768b19) into master (a3f2362) will decrease coverage by 6.46%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
- Coverage   56.70%   50.24%   -6.46%     
==========================================
  Files          46       46              
  Lines        3522     3519       -3     
==========================================
- Hits         1997     1768     -229     
- Misses       1525     1751     +226     
Impacted Files Coverage Δ
src/integrator_interface.jl 29.46% <ø> (-23.50%) :arrow_down:

... and 17 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

staticfloat commented 1 year ago

Ah, I didn't understand the original case properly. I didn't see any documentation that an AbstractSystem requires a name property, so I assumed it was optional. Perhaps this should be changed to a get_name() call, like the other properties?