Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix conservation tracking tests #2716

Merged
merged 7 commits into from
Jul 5, 2023

Conversation

btbest
Copy link
Contributor

@btbest btbest commented Jun 13, 2023

The six tests in testConservationTrackingHeadless.py have been xfailing for quite a while because file paths were broken. Having fixed that, it turned out also the main function import didn't work as implemented.

@btbest
Copy link
Contributor Author

btbest commented Jun 13, 2023

(see these six tests xfailing e.g. in this run: https://github.com/ilastik/ilastik/actions/runs/4698963212/jobs/8331947378 or most recently in https://app.circleci.com/pipelines/github/ilastik/ilastik/1340/workflows/130b9cc3-3761-43b7-bd3a-ea7609e32c4e/jobs/5647)

image

@btbest btbest changed the title Fix conservation tracking tests Draft: Fix conservation tracking tests Jun 13, 2023
@btbest
Copy link
Contributor Author

btbest commented Jun 13, 2023

Draft because (a) there's more tests with the same problem and (b) these shouldn't xfail when they can't find their input files, they should plain old fail.

But the first six are green now 🎉
image

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Patch coverage: 26.55% and project coverage change: +0.23 🎉

Comparison is base (fe83334) 54.35% compared to head (999d3ec) 54.59%.

❗ Current head 999d3ec differs from pull request most recent head 025785c. Consider uploading reports for the commit 025785c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2716      +/-   ##
==========================================
+ Coverage   54.35%   54.59%   +0.23%     
==========================================
  Files         519      530      +11     
  Lines       60863    61425     +562     
  Branches     8398     8453      +55     
==========================================
+ Hits        33081    33532     +451     
- Misses      26105    26140      +35     
- Partials     1677     1753      +76     
Impacted Files Coverage Δ
ilastik/applets/neuralNetwork/modelStateControl.py 0.00% <0.00%> (ø)
ilastik/applets/neuralNetwork/nnClassGui.py 0.00% <0.00%> (ø)
...lets/neuralNetwork/opNNClassificationDataExport.py 90.00% <ø> (ø)
...ets/trainableDomainAdaptation/modelStateControl.py 0.00% <0.00%> (ø)
...lets/trainableDomainAdaptation/tdaDataExportGui.py 0.00% <0.00%> (ø)
...leDomainAdaptation/trainableDomainAdaptationGui.py 0.00% <0.00%> (ø)
ilastik/shell/gui/ilastikShell.py 50.66% <ø> (ø)
lazyflow/operators/__init__.py 100.00% <ø> (ø)
lazyflow/operators/opCompressedUserLabelArray.py 90.32% <ø> (ø)
...ptation/_localTrainableDomainAdaptationWorkflow.py 27.11% <27.11%> (ø)
... and 15 more

... and 44 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@emilmelnikov
Copy link
Member

emilmelnikov commented Jun 13, 2023

A small tip: Github now supports draft pull requests, so it's not necessary to add [DRAFT] or Draft: to PR name.

@btbest btbest marked this pull request as draft June 13, 2023 20:54
Unclear why the previous rev (8710dfa) changed the import to `ilastik.__main__` in a bunch of other test files, but in this file went for a now broken `import main`. `ilastik.__main__` seems to work fine here too.
@btbest btbest force-pushed the fix_conservation_tracking_tests branch from 6b5a218 to 5849287 Compare June 14, 2023 14:19
@btbest
Copy link
Contributor Author

btbest commented Jun 14, 2023

I've found and fixed the remaining tests that had the same file path issues, but those tests suffer from their own additional problem: They contained a switch that would pass all tests if they are unable to import either multiHypoTracking_with_cplex or multiHypoTracking_with_gurobi; and that's why they were passing locally for me.

I changed the pass-if-missing to xfail-if-missing, and now all of the structuredLearning tests are xfailing on CI 😄

@emilmelnikov
Copy link
Member

I've found and fixed the remaining tests that had the same file path issues, but those tests suffer from their own additional problem: They contained a switch that would pass all tests if they are unable to import either multiHypoTracking_with_cplex or multiHypoTracking_with_gurobi; and that's why they were passing locally for me.

Instead of xfailing, we probably should conditionally skip those tests if a module cannot be imported.

@btbest btbest force-pushed the fix_conservation_tracking_tests branch from 5849287 to d1efdf9 Compare June 15, 2023 12:28
@btbest btbest force-pushed the fix_conservation_tracking_tests branch from 859c97f to 999d3ec Compare June 30, 2023 10:13
@btbest btbest changed the title Draft: Fix conservation tracking tests Fix conservation tracking tests Jun 30, 2023
@btbest btbest requested a review from k-dominik June 30, 2023 10:14
@btbest
Copy link
Contributor Author

btbest commented Jun 30, 2023

I managed to create a new input .ilp that passes (with Gurobi 9.5.2 on my system)

Turns out the GUI gives you no way to actually see all existing track labels that have been put in Training, and I only found the ones that weren't visible when I inspected the h5 manually.

On CI the tests will be skipped because no Gurobi there :)

@btbest btbest marked this pull request as ready for review June 30, 2023 10:17
@k-dominik
Copy link
Contributor

Hi @btbest,

sorry this took longer as I had to build the tracking with learning dependencies on apple silicon first (was pushing this off, so thanks for the nudge).

Two tests fail for me:

tests/test_ilastik/test_applets/structuredLearningTracking/testStructuredLearningTrackingHytraWithMergers.py::TestStructuredLearningTrackingHeadless::testStructuredLearningTrackingHeadless FAILED [ 50%]
tests/test_ilastik/test_applets/structuredLearningTracking/testStructuredLearningTrackingHytraWithMergers.py::TestStructuredLearningTrackingHeadless::testCSVExport FAILED                        [100%]

it appears that the outputs cannot be found, can you double check that in a clean repo, you can run these tests on windows?

e.g. I get an error wrt to not being able to find the following file:

ilastik/tests/test_ilastik/test_applets/structuredLearningTracking/../../data/inputdata/mitocheck_2d+t/mitocheck_small_2D+t_mergers-crop_data_Tracking-Result.h5

With the old .ilp file, the test errors out in vigra with "RandomForestn::predictProbabilities(): Too few columns in feature matrix". Probably because the file contains old feature names for the 2D convex hull location.
@btbest btbest force-pushed the fix_conservation_tracking_tests branch from 999d3ec to 025785c Compare July 5, 2023 12:50
@btbest
Copy link
Contributor Author

btbest commented Jul 5, 2023

🤦 I had "updated" the expected output file names to what seemed to be new default file names with -crop_data in the file name.

I have no idea how I obtained output files with that name pattern at any point, but anyway I've reverted the name change.

Copy link
Contributor

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

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

test success confirmed on my machine, too :) sooo good to have this running again!

@btbest btbest merged commit b001567 into ilastik:main Jul 5, 2023
10 of 11 checks passed
@btbest btbest deleted the fix_conservation_tracking_tests branch July 27, 2023 08:12
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