Hi,
Thanks for all your work in developing dinf! It's been interesting to see how ML models get adapted to demographic inference, and both pg-gan and dinf seem like big steps forward, especially incorporating posterior estimates.
Over the past week, I've been trying to get dinf working with a simple conda build, and ran into a few bugs that needed squashing to run the example models. In general, I've tested these from a conda install of this form (with conda-forge and bioconda already as default channels):
conda create -n dinf-gpu dinf bcftools "python<3.14"
This is pulling the following packages, amongst others:
bioconda/linux-64::bcftools-1.23.1-hb2cee57_0
bioconda/noarch::dinf-0.5.0-pyhdfd78af_0
bioconda/linux-64::htslib-1.23.1-h633afcb_0
conda-forge/noarch::jax-0.10.2-pyhd8ed1ab_0
conda-forge/linux-64::jaxlib-0.10.1-cuda129_py313h1656396_200
conda-forge/linux-64::numpy-2.5.0-py313hf6604e3_0
conda-forge/linux-64::python-3.13.14-h6add32d_100_cp313
With that installation, the simulation-only examples work perfectly fine. The real data examples, on the other hand, require the first two bug fixes.
Bug 1: 1000 Genomes metadata parsing
The first one was the biggest pain to troubleshoot, as the exception was thrown in a totally different section of the code. Using NumPy 2.0+, np.recfromtxt() has been removed from the main NumPy namespace, so this line:
|
data = np.recfromtxt(filename, names=True, encoding="ascii") |
needs to be replaced with:
data = np.genfromtxt(filename, names=True, dtype=['<U16', '<U16', '<U16', '<U16', 'i2', '<U16', '<U16'], encoding="ascii").view(np.recarray)
The first layer to that one is the obvious initial exception thrown about np.recfromtxt not existing. But when I replaced np.recfromtxt() with np.genfromtxt().view(np.recarray) without setting the dtype explicitly, dtype defaults to a float8, which then parses all the sample IDs and parental IDs as nans, which then produces empty self.samples in BagOfVcf, and cascades down to an AttributeError: 'NoneType' object has no attribute 'array' from this line:
|
a = variant.genotype.array() |
Specifying dtype=None also doesn't work, at least for 20130606_g1k_3202_samples_ped_population.txt, as there are a whole series of exceptions thrown for parsing columns into the wrong type. So the auto-detection of column types doesn't seem to work properly, at least for this file and invocation. I just threw together that explicit dtype list, but I'm pretty bad with dtype selection, so feel free to tweak it however you want.
Bug 2: dinf plot gan assumes truth values for parameters are set in model
Any of the example models that use real data generally don't specify a truth value when specifying the parameter ranges, yet the plotting code assumes one is present here:
|
if parameters is not None and x_param != "_Pr": |
|
truth = parameters[x_param].truth |
|
ax.axhline(truth, c="red", ls="-", alpha=0.7) |
So it's probably worth adding this to the conditional:
if parameters is not None and x_param != "_Pr" and parameters[x_param].truth is not None:
truth = parameters[x_param].truth
ax.axhline(truth, c="red", ls="-", alpha=0.7)
otherwise you get an exception like:
...
File "/home/pfr8/.conda/envs/dinf-gpu/lib/python3.13/site-packages/dinf/plot.py", line 1194, in __call__
ax.axhline(truth, c="red", ls="-", alpha=0.7)
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/pfr8/.conda/envs/dinf-gpu/lib/python3.13/site-packages/matplotlib/axes/_axes.py", line 820, in axhline
scaley = (yy < ymin) or (yy > ymax)
TypeError: '>' not supported between instances of 'float' and 'NoneType'
Bug 3: Python 3.14 Union dunder attributes are now read-only
This one's a bit silly, but these lines need to be commented out or reworked for compatibility with Python 3.14:
|
FeatureCollection.__doc__ = ( |
|
"""A feature array or a labelled collection of feature arrays.""" |
|
) |
See here for reference:
python/cpython#141829
Other minor suggestions
- There's an off-by-one error in the example
model.py files for any examples that use real data, since range() is right-exclusive, so range(21) should be range(22) here, for example:
|
keep_contigs={f"chr{c + 1}" for c in range(21)}, # Exclude chrX, etc. |
- Consider including bcftools in the suggested conda environment and Docker container in case the end user needs to convert their VCFs to BCF or wants to troubleshoot their inputs
- Documentation and usage still references
dinf.plot and dinf.tabulate rather than dinf-plot and dinf-tabulate, presumably due to these two lines:
|
prog="dinf.plot", description="Dinf plotting tools." |
|
prog="dinf.tabulate", description="Tabulate Dinf output." |
And a final note/curiosity:
It appears that neither BagOfVcf._close() nor BagOfVcf._reopen() is ever actually triggered, as you're using spawn rather than fork for your multiprocessing.Pool context, so the os.register_at_fork() call doesn't actually do anything (at least on our Linux cluster). As far as I can tell, everything works fine despite this, whether with -j 1 or -j 8.
All in all, thanks for developing dinf and providing such helpful documentation!
Best regards,
Patrick Reilly
Hi,
Thanks for all your work in developing dinf! It's been interesting to see how ML models get adapted to demographic inference, and both pg-gan and dinf seem like big steps forward, especially incorporating posterior estimates.
Over the past week, I've been trying to get dinf working with a simple conda build, and ran into a few bugs that needed squashing to run the example models. In general, I've tested these from a conda install of this form (with conda-forge and bioconda already as default channels):
conda create -n dinf-gpu dinf bcftools "python<3.14"This is pulling the following packages, amongst others:
With that installation, the simulation-only examples work perfectly fine. The real data examples, on the other hand, require the first two bug fixes.
Bug 1: 1000 Genomes metadata parsing
The first one was the biggest pain to troubleshoot, as the exception was thrown in a totally different section of the code. Using NumPy 2.0+,
np.recfromtxt()has been removed from the main NumPy namespace, so this line:dinf/dinf/vcf.py
Line 27 in 6a206ca
needs to be replaced with:
The first layer to that one is the obvious initial exception thrown about
np.recfromtxtnot existing. But when I replacednp.recfromtxt()withnp.genfromtxt().view(np.recarray)without setting thedtypeexplicitly,dtypedefaults to afloat8, which then parses all the sample IDs and parental IDs as nans, which then produces emptyself.samplesinBagOfVcf, and cascades down to anAttributeError: 'NoneType' object has no attribute 'array'from this line:dinf/dinf/vcf.py
Line 131 in 6a206ca
Specifying
dtype=Nonealso doesn't work, at least for20130606_g1k_3202_samples_ped_population.txt, as there are a whole series of exceptions thrown for parsing columns into the wrong type. So the auto-detection of column types doesn't seem to work properly, at least for this file and invocation. I just threw together that explicit dtype list, but I'm pretty bad with dtype selection, so feel free to tweak it however you want.Bug 2:
dinf plot ganassumes truth values for parameters are set in modelAny of the example models that use real data generally don't specify a
truthvalue when specifying the parameter ranges, yet the plotting code assumes one is present here:dinf/dinf/plot.py
Lines 1192 to 1194 in 6a206ca
So it's probably worth adding this to the conditional:
otherwise you get an exception like:
Bug 3: Python 3.14 Union dunder attributes are now read-only
This one's a bit silly, but these lines need to be commented out or reworked for compatibility with Python 3.14:
dinf/dinf/dinf_model.py
Lines 17 to 19 in 6a206ca
See here for reference:
python/cpython#141829
Other minor suggestions
model.pyfiles for any examples that use real data, sincerange()is right-exclusive, sorange(21)should berange(22)here, for example:dinf/examples/post_ooa/model.py
Line 19 in 6a206ca
dinf.plotanddinf.tabulaterather thandinf-plotanddinf-tabulate, presumably due to these two lines:dinf/dinf/plot.py
Line 1203 in 6a206ca
dinf/dinf/tabulate.py
Line 220 in 6a206ca
And a final note/curiosity:
It appears that neither
BagOfVcf._close()norBagOfVcf._reopen()is ever actually triggered, as you're usingspawnrather thanforkfor yourmultiprocessing.Poolcontext, so theos.register_at_fork()call doesn't actually do anything (at least on our Linux cluster). As far as I can tell, everything works fine despite this, whether with-j 1or-j 8.All in all, thanks for developing dinf and providing such helpful documentation!
Best regards,
Patrick Reilly