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

DataFrameDimension #807

Merged
merged 21 commits into from
Apr 4, 2020
Merged

DataFrameDimension #807

merged 21 commits into from
Apr 4, 2020

Conversation

jgrewe
Copy link
Member

@jgrewe jgrewe commented Mar 27, 2020

No description provided.

@achilleas-k
Copy link
Member

LGTM. Got a couple of issues for NIXPy out of this.

@achilleas-k
Copy link
Member

@gicmo Will you have a look at this or should I go ahead and merge?
Not sure what's up with codecov and if we should try to restart it somehow until it's finished.

@codecov
Copy link

codecov bot commented Apr 3, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@de63976). Click here to learn what that means.
The diff coverage is 88.39%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #807   +/-   ##
=========================================
  Coverage          ?   94.66%           
=========================================
  Files             ?      177           
  Lines             ?    12385           
  Branches          ?        0           
=========================================
  Hits              ?    11724           
  Misses            ?      661           
  Partials          ?        0
Impacted Files Coverage Δ
include/nix/base/IDataArray.hpp 100% <ø> (ø)
include/nix/base/IDimensions.hpp 100% <ø> (ø)
backend/hdf5/DimensionHDF5.hpp 100% <ø> (ø)
test/TestValidate.hpp 100% <ø> (ø)
test/BaseTestDataArray.cpp 100% <100%> (ø)
test/TestValidate.cpp 98.24% <100%> (ø)
backend/hdf5/DataArrayHDF5.cpp 89.22% <100%> (ø)
include/nix/valid/checks.hpp 100% <100%> (ø)
test/hdf5/TestDataArrayHDF5.hpp 100% <100%> (ø)
src/valid/validate.cpp 75.22% <100%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de63976...855fefb. Read the comment docs.

gicmo
gicmo reviewed Apr 3, 2020
View reviewed changes
backend/hdf5/DimensionHDF5.cpp Outdated
Comment on lines 364 to 365
if (col_index == -1)
return df.name();
Copy link
Member

Choose a reason for hiding this comment

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

I think, if we just move that above column_index = checkColumnIndex(col_index); we can get rid of the try-catch block. I am not a huge fan of using exceptions for control flow in C++.

gicmo
gicmo reviewed Apr 3, 2020
View reviewed changes
include/nix/DataArray.hpp Outdated
Comment on lines 431 to 433
std::vector<std::string> names = {column_name};
std::vector<unsigned> indices = frame.colIndex(names);
index = indices[0];
Copy link
Member

@gicmo gicmo Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There is unsigned colIndex(const std::string &name) const as a non-vector overload.

gicmo
gicmo reviewed Apr 4, 2020
View reviewed changes
.travis.yml
Comment on lines +31 to +34
apt:
update: true
packages:
- lcov
Copy link
Member

Choose a reason for hiding this comment

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

That is in theory not needed anymore, right @achilleas-k

Copy link
Member

Choose a reason for hiding this comment

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

Correct. Remove it.

Copy link
Member

Choose a reason for hiding this comment

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

No U!

gicmo
gicmo reviewed Apr 4, 2020
View reviewed changes
backend/hdf5/DimensionHDF5.cpp
}

nix::DataFrame df = dataFrame();
std::vector<Column> cols = df.columns();
Copy link
Member

Choose a reason for hiding this comment

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

Krass oversight in the DataFrame API that we have to fetch all the column information if all we want is the number. Filed as issue #808

gicmo
gicmo approved these changes Apr 4, 2020
View reviewed changes
Copy link
Member

@gicmo gicmo left a comment

Choose a reason for hiding this comment

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

Looks good. Some minor stuff we can fix in post :)

backend/hdf5/DimensionHDF5.cpp
boost::optional<unsigned> column_index = checkColumnIndex(col_index);

nix::DataFrame df = dataFrame();
std::vector<Column> all_cols = df.columns();
Copy link
Member

Choose a reason for hiding this comment

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

Also lame that we have to fetch all of them if we only want one. Filed as issue #809

@gicmo gicmo merged commit b3ad12e into G-Node:master Apr 4, 2020
@jgrewe jgrewe deleted the data_frame_dim branch April 4, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achilleas-k achilleas-k achilleas-k approved these changes

@gicmo gicmo gicmo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jgrewe @achilleas-k @gicmo