fit_map: r_pressure/w_pressure pressure-correction never selected — vfunc always resolves to _vkep/_vpow
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 82/100
Research direction
Start in eddy/rotationmap.py at verify_params_dictionary and inspect the existing _vkep_pressure and _vpow_pressure implementations. Compare the current selection logic with the behavior described for r_pressure, then add regression coverage in tests/test_rotationmap.py. Done means pressure parameters select the pressure-corrected function while existing non-pressure selection remains unchanged.
Written by the indexing model from the issue text.
Description
Summary
rotationmap.fit_map documents (and default_parameters.yml declares) a pressure-corrected
rotation-curve option via r_pressure/w_pressure, backed by working _vkep_pressure /
_vpow_pressure implementations. But verify_params_dictionary never selects them — params['vfunc']
is unconditionally set to _vpow or _vkep. Setting r_pressure currently has no effect on
the fitted model.
Version
eddy 3.1.1, eddy/rotationmap.py.
Where
# eddy/rotationmap.py, verify_params_dictionary, ~L1229
if params['vp_100'] is not None:
if params['mstar'] is not None:
params['vfunc'] = self._vpow
else:
raise ValueError("Cannot specify both `vp_100` and `mstar`.")
else:
params['vfunc'] = self._vkep
There is no branch checking params['r_pressure'] here, so _vkep_pressure (rotationmap.py:1579)
and _vpow_pressure (rotationmap.py:1605) are unreachable from fit_map's public API, despite
being actively maintained — see the jnp.where-vs-Python-if tracing comments in both functions,
added to fix a TracerBoolConversionError under the JIT/vmap path.
Regression
The selection logic used to exist. Commit 4631f4d ("included pressure term for outer disk",
2021-09-24) added it:
has_pressure = False if params.get('r_pressure') is None else True
...
if has_mstar:
if has_pressure:
params['vfunc'] = self._proj_vkep_pressure
else:
params['vfunc'] = self._proj_vkep
It was dropped in commit 5e336b5 ("adding option for self-gravity", 2022-06-06), which rewrote
verify_params_dictionary around the new disk-self-gravity (mdisk) feature and did not carry the
has_pressure branch forward. r_pressure/w_pressure have remained in default_parameters.yml
(current lines 143, 150) and the underlying velocity functions have kept being fixed (most recently
for JAX tracer correctness) ever since, so this reads as an accidental drop rather than an intentional
deprecation.
Reproduce
# rmap = some rotationmap with a fittable map
params = {'mstar': 0, 'vlsr': 1, 'r_pressure': 50.0, 'w_pressure': 10.0}
verified = rmap.verify_params_dictionary(params.copy())
print(verified['vfunc'].__name__) # '_vkep' -- not '_vkep_pressure'
Expected vs. actual
- Expected: when
r_pressure(and optionallyw_pressure) is set,fit_mapfits the
pressure-corrected rotation curve (_vkep_pressure/_vpow_pressure). - Actual:
vfuncalways resolves to the plain_vkep/_vpow;r_pressure/w_pressure
are silently ignored.
Suggested fix
In verify_params_dictionary, branch on params['r_pressure'] is not None the way 4631f4d
originally did, selecting _vkep_pressure/_vpow_pressure instead of _vkep/_vpow. Add a
regression test (tests/test_rotationmap.py has no coverage for r_pressure currently) so this
can't silently regress again.
- Dominant language
- Python
- Stars
- 23
- Forks
- 10
- Avg merge
- 28m
- Merged PRs (30d)
- 1
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from PlanetFormationLab/eddy
-
Difficulty 3/5 1-2 days Newbie friendliness 78/100
PlanetFormationLab/eddy#39 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 48/100
PlanetFormationLab/eddy#35 ·
-
Difficulty 5/5 Over a week Newbie friendliness 35/100
PlanetFormationLab/eddy#34 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 35/100
PlanetFormationLab/eddy#33 · 2 comments ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 48/100
PlanetFormationLab/eddy#32 ·
All issues in PlanetFormationLab/eddy
Similar issues
-
triage/confirmed
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
agentscope-ai/agentscope#2775 ·
-
comp/desktop P3 type/bug
Difficulty 1/5 Under an hour Newbie friendliness 92/100
NousResearch/hermes-agent#118866 ·
-
bug
Difficulty 1/5 Under an hour Newbie friendliness 90/100
apache/cloudstack#14222 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 82/100