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

Features/1550 extension of load npy from path to csv-files with same functionality #1551

Conversation

Reisii
Copy link
Collaborator

@Reisii Reisii commented Jul 2, 2024

Due Diligence

  • General:
  • Implementation:
    • unit tests: tested with and without input function
    • unit tests: multiple dtypes tested
    • documentation updated where needed

Description

Adds the same feature of PR#1388 now for .csv-files. Loads multiple .csv-files at once and concatenates them along an axis given as an input by the user. A function can be passed on as a parameter. The function would edit the pandas.Dataframe in way that it can be converted into a NumPy-Array without any problems

Issue/s resolved: #1550

Changes proposed:

Type of change

Memory requirements

Performance

Does this change modify the behaviour of other functions? If so, which?

no

@Reisii Reisii changed the title Features/1550 extension of load npy from path to csv f iles with same functionality Features/1550 extension of load npy from path to csv-files with same functionality Jul 2, 2024
Copy link
Contributor

github-actions bot commented Jul 4, 2024

Thank you for the PR!

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.06%. Comparing base (9d3af11) to head (956e3d0).
Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
heat/core/io.py 91.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1551      +/-   ##
==========================================
- Coverage   92.07%   92.06%   -0.01%     
==========================================
  Files          83       83              
  Lines       12144    12180      +36     
==========================================
+ Hits        11181    11214      +33     
- Misses        963      966       +3     
Flag Coverage Δ
unit 92.06% <91.66%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mrfh92 mrfh92 self-requested a review July 4, 2024 20:19
heat/core/io.py Outdated Show resolved Hide resolved
heat/core/io.py Outdated Show resolved Hide resolved
heat/core/io.py Show resolved Hide resolved
heat/core/io.py Outdated Show resolved Hide resolved
@mrfh92 mrfh92 added the PR talk label Jul 8, 2024
@mrfh92
Copy link
Collaborator

mrfh92 commented Jul 8, 2024

discussed in the PR talk: we will make pandas an optional dependency, as already the case for h5py etc.

…les_with_same_functionality' of github.com:helmholtz-analytics/heat into features/1550-Extension_of_load_npy_from_path_to_csv-fIles_with_same_functionality
Copy link
Contributor

github-actions bot commented Jul 9, 2024

Thank you for the PR!

Copy link
Contributor

github-actions bot commented Jul 9, 2024

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

@Reisii
Copy link
Collaborator Author

Reisii commented Jul 15, 2024

@Reisii something failed on the CUDA-runner. Can you reproduce this on our workstation?

So it works perfectly fine with the workstation, I'll just try to test out using ht.MPI_WORLD.Barrier() on different places

Copy link
Contributor

Thank you for the PR!

@mrfh92 mrfh92 self-requested a review July 15, 2024 14:03
@mrfh92 mrfh92 dismissed their stale review July 16, 2024 06:44

just done to let CI run

…les_with_same_functionality' of github.com:helmholtz-analytics/heat into features/1550-Extension_of_load_npy_from_path_to_csv-fIles_with_same_functionality
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

@mrfh92
Copy link
Collaborator

mrfh92 commented Aug 13, 2024

lines 1187/88, 1268/87 are not covered by tests so far. It looks like that there is no remainder in the devision (?)
That lines 1203-1209 are not covered can't be resolved as this would require a different setup without h5py, which is currently out of scope

…les_with_same_functionality' of github.com:helmholtz-analytics/heat into features/1550-Extension_of_load_npy_from_path_to_csv-fIles_with_same_functionality
Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Collaborator

@mrfh92 mrfh92 left a comment

Choose a reason for hiding this comment

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

Changes look fine :)
Thanks for your work 👍

Copy link
Contributor

Thank you for the PR!

@mrfh92 mrfh92 merged commit cbdf49a into main Aug 29, 2024
42 checks passed
@mrfh92 mrfh92 deleted the features/1550-Extension_of_load_npy_from_path_to_csv-fIles_with_same_functionality branch August 29, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension of load_npy_from_path() to .csv-fIles with same functionality
2 participants