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

586 sic sut mapping #113

Merged
merged 6 commits into from
Oct 22, 2024
Merged

586 sic sut mapping #113

merged 6 commits into from
Oct 22, 2024

Conversation

Jday7879
Copy link
Collaborator

Summary

Functions created to validate mapping files against input data. will produce a warning if there is unmatched data in the dataframe. No warning or errors if unused mappings are present in the mapping file

Checklists

This pull request meets the following requirements:

  • installable with all dependencies recorded
  • runs without error
  • follows PEP8 and project specific conventions
  • appropriate use of comments, for example no descriptive comments
  • functions documented using Numpy style docstings
  • assumptions and decisions log considered and updated if appropriate
  • unit tests have been updated to cover essential functionality for a reasonable range of inputs and conditions
  • other forms of testing such as end-to-end and user-interface testing have been considered and updated as required
  • tests suite passes (locally as a minimum)
  • peer reviewed with review recorded

If you feel some of these conditions do not apply for this pull request, please
add a comment to explain why.

@AntonZogk AntonZogk requested review from AntonZogk and removed request for AntonZogk October 17, 2024 10:57
@Jday7879 Jday7879 requested a review from lhubbardONS October 21, 2024 10:27
Copy link
Collaborator

@lhubbardONS lhubbardONS left a comment

Choose a reason for hiding this comment

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

Function looks good and performs required task, test and pre-commit hooks pass. Just one minor optional comment left in-line - happy to discuss further. Otherwise, happy to approve merge to main!

}

for i in files_and_column_names:
mapping_path = mapping_folder + i
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comment: worth having something in the docstring to say the mapping_folder needs to have a '/' at the end for a readable file path? (or validation?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the doc string to be more explicit about this, nice spot!

@Jday7879 Jday7879 merged commit cc1cde3 into main Oct 22, 2024
3 checks passed
@Jday7879 Jday7879 deleted the 586-sic-sut-mapping branch October 22, 2024 08:43
AntonZogk pushed a commit that referenced this pull request Oct 28, 2024
* Creating mapping validation function

* Docstrings

* Update docstring and leave unmatched as set

* Update to raise warning and created wraper to test multiple mapping files in one go

* adding test that passes when a warning raised

* update docstring to ask that mapping file be a folder and not a file
AntonZogk added a commit that referenced this pull request Oct 29, 2024
* Build pbj-workbench-python3.9-standard.Dockerfile and test

* Check files

* Build full path instead of cd

* Test whole process with 3.8

* Update push branch for testing

* Navigate to project folder

* Add ls for debugging

* docker run -t cml_3.9

* Run docker in detached mode

* Run docker in -i mode

* Split process to many steps

* Add matrix with cml versions

* Typo matrix

* Tupo matrix

* Typo matrix

* Use string type for versions

* continue-on-error: true

* Tidy up workflow

Break steps
Add 3.11 runtime
Change name tags

* Set wd in pytest

* Add pre commit hooks

* Use checkout v3

* Move files around with docker cp

* Fix typo

* Use checkouts

* Copy files to container

* Mount parent volume

* Create container after checkout

* Trigger on pull requests in main

* 586 sic sut mapping (#113)

* Creating mapping validation function

* Docstrings

* Update docstring and leave unmatched as set

* Update to raise warning and created wraper to test multiple mapping files in one go

* adding test that passes when a warning raised

* update docstring to ask that mapping file be a folder and not a file

* Correct design and calibration values (#108)

*Add reusable function
*Add test data
*Add unit test
*Add TODO to use this function in other parts of the pipeline

* 632 module restructure (#116)

* move files to correct location

* change relevant module imports

* Move files to apropiate folders, rename folders

* Move all data to equivalent test folder structure

* Update estimation test paths and imports

* Update imputation test paths and imports

* Update outlier detection test paths and imports

* Update outpus test paths and imports

* Update utilities test paths and imports

* Remove tests/imputation/test_pivot_imputation_value.py

* Run hooks

* Add tests tree into tests readme

* Run hooks

* Remove duplicated test data

* These were copied instead of moved, hence duplicated

* these files aren't needed and covered by other tests

* update tree

---------

Co-authored-by: Wil Roberts <[email protected]>

* Use list, passing dict not support in pandas 2.1.4

* Enforce constrain marker str type in tests

* Create separate job for pre commit hooks

* Use legacy job for hooks

* Run hooks

* Pre commit mig config, user python instead of python3

* Use python 3.9 for hooks

* Use 3.10 for hooks

* Use python 3.10.13

---------

Co-authored-by: Jordan-Day-ONS <[email protected]>
Co-authored-by: Wil Roberts <[email protected]>
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.

2 participants