Changes to test files from PR #876 #880
Conversation
There was a problem hiding this comment.
From @ns-rse:
Not sure this file needs to be here 🤔
My understanding is that the tests/resources/ministats.topostats file is one that is compared to the output that is written to the tmp_path/ directory when running TopoStats. I can't see that this is read in anywhere (minicircle.topostats.out is but I've suggested moving and renaming that to reflect the test it relates to).
There was a problem hiding this comment.
From @ns-rse:
I think this file shouldn't be under _regtest_outputs/ as this is a directory that pytest-regtest writes the output of the print() statement to.
Its also not clear how or why it differs from tests/resources/processed/minicircle.topostats.
I would expect there to be one target HDF5 file in the tests that are compared to what is produced by a full run. If there is need to have different versions then I would place them under tests/resources/ with a name that makes it clear what aspect they relate to (in this instance nodestats so something like tests/resources/minicircle_nodestats.topostats).
| ) | ||
|
|
||
| BASE_DIR = Path.cwd() | ||
| REGRESSION = BASE_DIR / "tests/nodestats-regression" |
There was a problem hiding this comment.
| REGRESSION = BASE_DIR / "tests/nodestats-regression" | |
| RESOURCES = BASE_DIR / "tests/resources" |
| with h5py.File(tmp_path / "tests/resources/processed/minicircle.topostats", "r") as f: | ||
| saved_topostats = hdf5_to_dict(f, group_path="/") | ||
|
|
||
| with h5py.File(REGRESSION / "minicircle.topostats.out", "r") as f: |
There was a problem hiding this comment.
| with h5py.File(REGRESSION / "minicircle.topostats.out", "r") as f: | |
| with h5py.File(RESOURCES / "minicircle_nodestats.topostats", "r") as f: |
|
Thanks @llwiggins is this ready for review? I also wonder if it might make more sense to merge this into the |
|
Hi @ns-rse, @MaxGamill-Sheffield is going to take a look at this next week. There are some additional changes to be made so best to keep this as a draft PR for now I think! |
|
Hey @ns-rse, this test is now very outdated as both the structure and saved keys of the disordered tracing, nodestats, and ordered tracing stats in the .topostats file differs greatly. The intention of this test was just to ensure that of the keys that were saved, they were the same but now this needs more manual coercion of the dictionaries which will be done once the final .topostats structure is saved (upon finishing the final splining section). And this PR & branch then serves to save those outputs somewhere findable while keeping the individual section PR's / branches solely focused on their features. This PR never has any intention of being merged with main or 800-btr as it should already be covered in the main tests, again, it is here just to not break the working tests and save a temporary output. |
Can we close this PR then? If its never going to be merged the work can remain on a branch. |
Bump...if the PR isn't going to be merged it doesn't really serve any purpose. The changes persist on the branch and can be referenced if needed, can we therefore close it? |
This PR isolates the changes to tests made in PR #876 so that they can be reviewed/amended separately to the changes to the main tracing code. Suggestions in the comments from @ns-rse have been included here so that they can be addressed before merging to
maxgamill-sheffield/800-better-tracing.