Skip to content

changed regressions and weights tests to use API#480

Merged
Zaharid merged 4 commits into
masterfrom
teststoapi
Jun 11, 2019
Merged

changed regressions and weights tests to use API#480
Zaharid merged 4 commits into
masterfrom
teststoapi

Conversation

@wilsonmr
Copy link
Copy Markdown
Contributor

@wilsonmr wilsonmr commented Jun 6, 2019

closes #459

There are a couple of tests which I wanted to show can produce the same numbers in this commit but I think instead of creating a pd.DataFrame which we take values from to create a different one, the CSVs for relevant tests could just be changed to be the original CSVs (this would just be changing columns/indices) - These are marked with a TODO and should be done before merging

Most of the tests become simpler, there is the caviat that the fits test needs a work around which kind of defeats the point of the API but I felt like deleting as much as I could from conftest.py criticism welcome..

@wilsonmr wilsonmr requested review from Zaharid and siranipour June 6, 2019 12:09
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jun 6, 2019

Looks good to me at first sight.

columns=map(str,
range(th.shape[1])))
def test_predictions(data_config):
#TODO: change the baseline to just be the `experiment_result_table`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't understand what is the problem with doing this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so if I were to have:

    exp_res_tab = API.experiment_result_table_no_table(**data_config)
    return exp_res_tab.iloc[:, 2:]

then the indexes don't match. I just wanted to make sure that the tables matched before changing the baseline table... Although now that I think about it, it would have been apparent on the git diff anyway..

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The solution to that is really #449 in some form...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, actually I played with it for a bit and I think this might be the best solution for now, because the tableloader which saves the table sane_load by default needs 1 level of indexing and so to mess around with making new indexes is more effort than to just take the values and construct a new dataframe as is being done here.

Maybe I should expand the comment slightly but then leave it as is?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, let's do that.

…d reconstruct a pd.DataFrame due to limitations of CSV
@wilsonmr
Copy link
Copy Markdown
Contributor Author

wilsonmr commented Jun 7, 2019

Ok I think I'm happy with this

covs = [results.experiment_covariance_matrix(exp, False, None) for exp in exps]
return covs
def data_witht0_config():
experiment_list = [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make sense to move the common experiment_list outside?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah actually these configs are all copy pastas which is probably unneccessary I'll trim it down

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is that better?

@wilsonmr
Copy link
Copy Markdown
Contributor Author

wilsonmr commented Jun 7, 2019

oops there are some unused imports now

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jun 11, 2019

Hello, this is @Zaharid's automated QA script. Please note it is highly experimental. I ran pylint on your changes and found some new issues.

On validphys2/src/validphys/tests/test_regressions.py, pylint has reported the following new issues:

  • Line 80: TODO: ideally we would change the baseline to just be corresponding columns
  • Line 90: TODO: As in test_predictions
  • Line 97: TODO: As in test_predictions

@Zaharid Zaharid merged commit f9ce230 into master Jun 11, 2019
@Zaharid Zaharid deleted the teststoapi branch June 11, 2019 10:26
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.

Should validphys tests use new API

2 participants