fit_map: r_pressure/w_pressure pressure-correction never selected — vfunc always resolves to _vkep/_vpow

Open Beginner friendly
#36 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
82/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Quiet
Tech stack
python
Domain
backend

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 optionally w_pressure) is set, fit_map fits the
    pressure-corrected rotation curve (_vkep_pressure/_vpow_pressure).
  • Actual: vfunc always 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from PlanetFormationLab/eddy

All issues in PlanetFormationLab/eddy

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.