MCST/AREPO: correct code_time, register defaults, silence false mismatches#237
Merged
Conversation
…tches
Drive-by fixes from triaging a user-reported "missing units" bug on MCST
runs at MPCDF:
- units/gadget_cosmological.yaml:
* code_time: 0.978 * Gyr -> kpc / (km/s). Pint's Gyr is not Julian-year
* 1e9 here, so the previous form evaluated to 3.11474e+16 s instead
of the AREPO-derived 3.085678e+16 s -- 0.94% high, squaring to 1.89%
on energy/pressure (visible as BH_CumEgyInjection_QM/RM mismatches).
* IonisingPhotonRate1e49: add override:true. Some MCST AREPO builds
store to_cgs as the reciprocal (1e-49); unit file is authoritative.
* BH_Mdot{,Bondi,Eddington}: add override:true. AREPO writes inverted
scaling attrs (length=1,mass=-1,velocity=1 -> L^2/(M*T)) while the
cgs prefactor is correct M/T; trust the unit file's dimensions.
* Add MCST per-subhalo Subhalo{H2,H,He}Mass entries.
- interfaces/mixins/units.py: check_unit_mismatch now treats 'none' in
the unit file as equivalent to a dimensionless metadata Quantity with
cgs factor 1. Eliminates a false-positive class for integer ID/counter
fields (StromgrenSourceID, NumberOfSupernovae, ...) where the writer
emits to_cgs=1 + zero scalings.
- configfiles/config.yaml: add /vera/ptmp/gc/MCST to stock datafolders
next to the other MPCDF paths so MCST runs load by relative name.
- tests/testdata.yaml: register MCST_ST15_{snapshot,group}_z5p5 (new z=5.5
variant on disk alongside the existing ST8 z=3 entries).
Verified end-to-end: ST8 and ST15 snapshot+groupcat each load with zero
non-success unit states using stock scida defaults.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Triaging a user-reported "missing units" bug on MCST simulations at MPCDF surfaced a small cluster of independently-actionable defects in stock scida defaults. All fixes verified end-to-end on the MCST_ST8 (z=3) and MCST_ST15 (z=5.5) test snapshots: 0 non-success unit states across snapshot + groupcat for both variants.
Changes
units/gadget_cosmological.yamlcode_time:0.978 * Gyr->kpc / (km/s). pint'sGyris not Julian-year x 1e9 here, so the previous form evaluated to3.11474e+16 sinstead of the AREPO-derived3.085678e+16 s-- 0.94% high, squaring to 1.89% on anything quadratic incode_time(energy, pressure). This surfaced asBH_CumEgyInjection_QM/RMmismatches.IonisingPhotonRate1e49:override: true. Some MCST AREPO builds write theto_cgsattr as the reciprocal (1e-49instead of1e+49) -- a sign-inversion bug in the writer's dimensional path for fields whose name encodes the scale. The unit file is authoritative.BH_Mdot{,Bondi,Eddington}:override: true. AREPO writes dimensionally inverted scaling attrs (length_scaling=1, mass_scaling=-1, velocity_scaling=1->L^2/(M*T)) whileto_cgscarries the correctM/Tmagnitude. Trust the unit file's dimensions.Subhalo{H2,H,He}Massentries.interfaces/mixins/units.pycheck_unit_mismatchnow treats'none'in the unit file as equivalent to a dimensionless metadataQuantitywithcgsfactor1. Eliminates a false-positive class for integer ID / counter fields (StromgrenSourceID,NumberOfSupernovae, ...) whose writer emitsto_cgs=1and zero scalings.configfiles/config.yaml/vera/ptmp/gc/MCSTto stockdatafolders, alongside the other MPCDF paths. MCST runs are loadable by run-name without per-user config.tests/testdata.yamlMCST_ST15_{snapshot,group}_z5p5next to the existing ST8 z=3 entries, picking up the new z=5.5 variant already on disk.Test plan
MCST_ST8_snapshot_z3loads with zero non-success unit states (default unit file path).MCST_ST8_group_z3loads with zero non-success unit states.MCST_ST15_snapshot_z5p5loads with zero non-success unit states.MCST_ST15_group_z5p5loads with zero non-success unit states.check_unit_mismatchdirect unit tests:'none' vs Q(1, dimensionless)returns True;'none' vs Q(2, dimensionless)and'none' vs Q(1, km)still log mismatches.code_timefix,code_time / (UnitLength_in_cm / UnitVelocity_in_cm_per_s) = 1.000000exactly.Generated with Claude Code