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 tests #33

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

faysal-ishtiaq
Copy link
Contributor

No description provided.

@faysal-ishtiaq faysal-ishtiaq marked this pull request as draft December 5, 2022 19:06
@faysal-ishtiaq
Copy link
Contributor Author

@isedwards there is one more failing test which is not related to plots but a table. This is the failing test

FAILED tests/test_cdms_products.py::test_inventory_table - AssertionError: assert False

I have checked the generated and actual files manually, and they do not look the same.

You can reproduce the result by running pytest

Then you can put this file https://github.com/faysal-ishtiaq/opencdms-process/blob/rpy2Test/tests/data/daily_niger.csv
and generated file ( tests/results_actual/inventory_table_actual010.csv ) side by side and see the difference. Looking at the files, it seems that the bug's origin is in cdms.products.

@lloyddewit
Copy link
Contributor

@faysal-ishtiaq If it helps, then I could look at results_actual/inventory_table_actual010.csv to see if I have an idea what's causing the difference.

@isedwards
Copy link
Collaborator

@faysal-ishtiaq If it helps, then I could look at results_actual/inventory_table_actual010.csv to see if I have an idea what's causing the difference.

@lloyddewit, yes please - let us know if you can see what is causing the difference

click.echo("Replace this message by putting your code into "
"opencdms_process.cli.main")
click.echo(
"Replace this message by putting your code into " "opencdms_process.cli.main"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was the formatting change here accidental? It looks a bit odd being >79 chars and also the unnecessary concatenation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was due to black, not formatting strings. I have added a make command in the makefile with a preview flag for black so that black takes care of strings as well.

@faysal-ishtiaq
Copy link
Contributor Author

@lloyddewit

Here is the generated csv file

inventory_table_actual010.csv

@isedwards
Copy link
Collaborator

@lloyddewit

Here is the generated csv file

inventory_table_actual010.csv

@lloyddewit did you get a chance to look at what may be causing the difference with the inventory table?

@lloyddewit
Copy link
Contributor

@isedwards @faysal-ishtiaq
I've been sick the last 2 days. Sorry for the slow response.
The difference is due to how null values are displayed in the csv file. In the linked file, they are shown as 'NA_character_' whereas in the expected results file they are shown as empty strings. The other values all look correct.
I'll investigate further when I'm recovered, I hope that's OK. Apologies again for the inconvenience.

@lloyddewit
Copy link
Contributor

@isedwards @faysal-ishtiaq
I ran the inventory_table test directly in the R environment. The results were functionally the same as the linked file and the expected results in the repo. The only differences are how months with less than 31 days are represented (February, April, June etc.). For example 31 April is shown as below in the R environment, expected results file and linked file respectively:

image

image

image

I suspect that our development environments are behaving slightly differently in the line below:

image

In summary, I think your results are correct as far as the user is concerned. The test comparison fails because missing values are written differently to the csv file (or potentially represented differently in the pandas data frame).

In the context of your current work, how important is this?

This code requires Python, R and many third-party R/Python packages. The dependencies are complex and it's hard to get them identical in different development environments. In the future, should we consider having a reference installation and use this to run the unit tests?

@isedwards
Copy link
Collaborator

Just for info: the name of this repo recently change from github.com/opencdms-processes/opencdms-process to github.com/opencdms-components/opencdms-rclimatic in order to reflect that this repo will be a component/plugin for OpenCDMS for working with the cdms.products R Climatic library. The old links will continue to redirect to the new repo, so you can continue to work with the old name if this helpful.

@faysal-ishtiaq faysal-ishtiaq marked this pull request as ready for review December 21, 2022 16:07
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