Skip to content

Conversation

@rhugman
Copy link
Contributor

@rhugman rhugman commented Nov 10, 2025

Added AutoEncoder DSI Class.
Requires tensorflow (optional).

@coveralls
Copy link

coveralls commented Nov 10, 2025

Coverage Status

coverage: 76.367% (-1.6%) from 77.941%
when pulling e38097b on rhugman:feat_dsiae
into 765bf96 on pypest:develop.

@rhugman
Copy link
Contributor Author

rhugman commented Nov 11, 2025

@briochh and @jtwhite79 - I think (??) im loosing coverage because of skipping dsiae tests to avoid installing tensorflow. Not sure how best to handle that...will adding tensorflow to the test .env be inconvenient?

@briochh
Copy link
Collaborator

briochh commented Nov 11, 2025

@rhugman, at the moment the testing environment is taken from the environment.yml file in etc .see how things go if you stick tensorflow in there, it should pick it up and run your tests.
That yml is only really used in the CI so having it in the pyproject.toml under optional is good for a project dependancy point of view.

I have been battling with exploring the use of uv on the CI which would allow us to just maintain that toml file but too many pain points at the moment with intersecting os-types, python versions, numpy/blas bugs(?).

On a tests note, it'd be good to make sure these additions are truly independent (the is no guarantee of run order on the CI) and that they don't hit files on the same path. For all its additional complexity pytest does help sand box tests so that they don't interfere (using the tmp_path arg). I'll comment inline...

@pytest.mark.skipif(not HAS_TENSORFLOW, reason="TensorFlow not available")
def test_dsiae_basic():
"""Test basic DSIAE functionality without transforms"""
dsiae = dsiae_freyberg_basic(tmp_d="temp_dsiae_basic")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be ok as long as nothing else tries to write to "temp_dsiae_basic". If we use a tmp_path arg here pytest will use a unique temporary directory... you can then pass this through to the receiving function.
If you are running locally from __main__ (i.e. outside pytest) then you can pass an argument value of your choosing in the call string (e.g. "temp").
If you are running locally using pytest you can use the flag --basetemp in your command line call (e.g. pytest autotest/emulator_tests.py::test_dsiae_basic --basetemp=runner -vv -s etc)

def test_dsiae_auto_latent_dim():
"""Test DSIAE with automatic latent dimension selection"""

test_d = "ends_master"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are going to have issues if "ends_master" does not exist. Which might be the case as tests are run independently.
I haven't quite worked out the best way to deal with dependency (especially if running in parallel). There may be options with artefacts and or test classes but the easiest way might be to combine the dependent tests (I know, creating even an even longer test).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these emulator tests are just re-using the freyberg setup because i was being lazy (and because i was using freyberg to validate the implementation...).

I could in principle just create a dummy synthetic dataset and generate a base Pst on the fly. Would this be preferable? Then we should be able to run all the tests independetly

@briochh
Copy link
Collaborator

briochh commented Nov 11, 2025

@rhugman, see how you go merging develop back into your fork. The updated tests are running a bit quicker so maybe not all waisted effort at this end!

@briochh briochh merged commit f07b709 into pypest:develop Dec 24, 2025
13 checks passed
@briochh
Copy link
Collaborator

briochh commented Dec 24, 2025

Nice one @rhugman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants