Skip to content

#455 Created reference testing values file #552

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.

Sign up for GitHub

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

Jump to bottom
Merged
jan-petr merged 8 commits into from
Jun 16, 2021
Merged

#455 Created reference testing values file #552

jan-petr merged 8 commits into from
Jun 16, 2021

Conversation

@MDijsselhof
Copy link
Contributor

@MDijsselhof MDijsselhof commented May 7, 2021

closes #455

@MDijsselhof MDijsselhof requested a review from jan-petr May 7, 2021 14:26
@MDijsselhof MDijsselhof self-assigned this May 7, 2021
@MDijsselhof MDijsselhof linked an issue May 7, 2021 that may be closed by this pull request
Reference testing values of xASL_qc_TestExploreASL #455
Closed
jan-petr
jan-petr requested changes May 8, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Not really what I had on my mind - sorry ;)

I meant save those values in a easy-to-read format for Matlab - i.e. TSV or MAT file, so that it can be read and parsed by Matlab.

Then in function xASL_qc_TestExploreASL in section 8 (line 465), it will read this reference table from a fixed location. And compare with the results table calculated here and then output either 1 or 0 if the difference in all fields is below 0.1% or not.

The aim is that we don't have to compare the old and new results by eye, but automatically. And that we have a fixed saved reference.

@MichaelStritt MichaelStritt added the testing Either unit testing or full pipeline testing for bugs label May 12, 2021
@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 15, 2021

Hey guys, I didn't want to interfere here, but maybe we can close this PR before v1.7.0 because the branch is already behind by a lot of commits.

@MichaelStritt MichaelStritt requested a review from jan-petr June 15, 2021 14:43
@jan-petr
Copy link
Contributor

jan-petr commented Jun 15, 2021

Hey guys, I didn't want to interfere here, but maybe we can close this PR before v1.7.0 because the branch is already behind by a lot of commits.

Great Michael. Have you tested that this works? I will re-run and test everything...

@jan-petr
Copy link
Contributor

jan-petr commented Jun 15, 2021

  1. you provide also ExploreASL version and OS in the table. I would like to make it possible to add results from different ExploreASL versions, and it will always compare the results for the current and for all version
  2. checking for all lines of the results if they are similar between the current and old version.

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 16, 2021

The comparison part is still missing, but I can write that this morning. If you want to keep on updating this, maybe a .mat file will be easier than a .tsv file.

@jan-petr
Copy link
Contributor

jan-petr commented Jun 16, 2021

The comparison part is still missing, but I can write that this morning. If you want to keep on updating this, maybe a .mat file will be easier than a .tsv file.

OK. Please do so. I do like the TSV file actually. Especially the fact that it is human readable. I would keep it, it can be nicely and automatically read, I don't see a huge disadvantage in it...

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 16, 2021

@jan-petr: It works for me on windows now :)
It only compares to the most up-to-date version, but can read multiple versions. It saves a "comparison table" as well as the reference table.

jan-petr
jan-petr requested changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Perfect. I get the difference table. And prints the xASL version, but can I also get a simple 1/0 passed not passed - i.e. checking if all values are below 0.01% or something. Also if both tables have "n/a" entry, then the error should be 0%, otherwise, with single n/a it should be 100%.

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 16, 2021

Test with 3 versions

...
Version: xASL_v1_6_0
OS:      Linux_2016b
Version: xASL_v1_6_1
OS:      Linux_2016b
Version: xASL_v1_6_2
OS:      Linux_2016b
...
Version: xASL_v1_6_0
Passed:  false
Version: xASL_v1_6_1
Passed:  false
Version: xASL_v1_6_2
Passed:  false
...

@MichaelStritt MichaelStritt requested a review from jan-petr June 16, 2021 17:56
jan-petr
jan-petr approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

OK. Increased the tolerance to 1%, 0.1% was too optimistic :)
Also ignoring strings...

@MichaelStritt MichaelStritt force-pushed the feature-#455_reference_testing_values branch from 7ac7162 to f0e250a Compare June 16, 2021 18:15
@jan-petr jan-petr force-pushed the feature-#455_reference_testing_values branch from f0e250a to 7ac7162 Compare June 16, 2021 20:11
@jan-petr jan-petr force-pushed the feature-#455_reference_testing_values branch from 7ac7162 to 23fa24c Compare June 16, 2021 20:14
@jan-petr jan-petr merged commit 23fa24c into develop Jun 16, 2021
@jan-petr jan-petr deleted the feature-#455_reference_testing_values branch June 16, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jan-petr jan-petr jan-petr approved these changes

Assignees

@jan-petr jan-petr

Labels

testing Either unit testing or full pipeline testing for bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reference testing values of xASL_qc_TestExploreASL

4 participants

@MDijsselhof @MichaelStritt @jan-petr