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.

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

Update testdata-minimal scripts #691

Draft
wants to merge 8 commits into
base: main-dev
Choose a base branch
from
Draft

Update testdata-minimal scripts #691

wants to merge 8 commits into from

Conversation

avaldebe
Copy link
Collaborator

@avaldebe avaldebe commented Jul 4, 2022

close #604

@avaldebe avaldebe added this to the v0.13.2 milestone Jul 4, 2022
@avaldebe avaldebe self-assigned this Jul 4, 2022
@codecov
Copy link

codecov bot commented Jul 4, 2022 •
edited
Loading

Codecov Report

Merging #691 (513cf53) into main-dev (c419649) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           main-dev     #691   +/-   ##
=========================================
  Coverage     80.04%   80.04%           
=========================================
  Files            97       97           
  Lines         17422    17422           
=========================================
  Hits          13946    13946           
  Misses         3476     3476
Flag Coverage Δ
unittests 80.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@avaldebe avaldebe requested review from jgriesfeller and removed request for jgriesfeller July 5, 2022 14:13
@avaldebe avaldebe mentioned this pull request Jul 5, 2022
Closed
@avaldebe
Copy link
Collaborator Author

avaldebe commented Jul 5, 2022

Hi @jgriesfeller

I would like some feedback hare. Specially regarding on the scope of the PR.

I think that I could push a bit further and combine all the scripts into one CLI app,
that creates all the files required and put them into date stamped tar file.
What do you think?

@avaldebe
Copy link
Collaborator Author

avaldebe commented Jul 6, 2022

@jgriesfeller

It looks to me that scripts/testdata-minimal/TM5_subset.sh is not used when creating the test dataset.
It is supposed to be run on the test files directly, so I do not know where the original files are located.

Can you think of any way to check if it was used on the test files?

jgriesfeller
jgriesfeller reviewed Jul 7, 2022
View reviewed changes
scripts/testdata-minimal/create_subsets_ghost.py Outdated
assert path_in.is_dir(), f"missing {path_in}"
assert path_out.is_dir(), f"missing {path_out}"

datasets = ["EEA_AQ_eReporting", "EBAS"]
Copy link
Member

Choose a reason for hiding this comment

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

EEA_AQ_eReporting is an old intermediate dataset that will not change anymore and is now part on the actual EEA data.
In principle not needed anymore.
I will write an issue to remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I can remove it now. Just need the issue number for reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I can remove it now. Just need the issue number for reference.

or I just remove it now...

jgriesfeller
jgriesfeller approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@jgriesfeller jgriesfeller left a comment

Choose a reason for hiding this comment

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

all good

@jgriesfeller
Copy link
Member

These were just some quick and dirty creation scripts for the testdata-minimal. I moved them into the repo after some problems to find them after EBAS had changed its data format some months back. They have been part of the testdata-minimal tar file before.
So I think they should be in the repo, but we should not spent a great amount of time on them. So I am fine with the current structure. But I will also support if you decide to do them right.

@avaldebe
Copy link
Collaborator Author

avaldebe commented Jul 7, 2022

These were just some quick and dirty creation scripts for the testdata-minimal. I moved them into the repo after some problems to find them after EBAS had changed its data format some months back. They have been part of the testdata-minimal tar file before. So I think they should be in the repo, but we should not spent a great amount of time on them. So I am fine with the current structure. But I will also support if you decide to do them right.

I'll add usage instructions to the README before merging

@avaldebe avaldebe marked this pull request as draft July 7, 2022 12:14
@avaldebe
Copy link
Collaborator Author

avaldebe commented Jul 7, 2022

I'm going to spend a bit of time over the next week cleaning up and testing the scripts.

@lewisblake lewisblake linked an issue Jul 8, 2022 that may be closed by this pull request
Testing suite should be machine and user agnostic #692
Closed
@avaldebe avaldebe modified the milestones: v0.13.2, 2023.04 Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgriesfeller jgriesfeller jgriesfeller approved these changes

Assignees

@avaldebe avaldebe

Labels
testing
Projects
None yet
Milestone
v0.14.0
2 participants
@avaldebe @jgriesfeller