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] table_to_frame: metas lost on conversion #4259

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

itslarin
Copy link
Contributor

@itslarin itslarin commented Dec 7, 2019

Issue
Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@claassistantio
Copy link

claassistantio commented Dec 7, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 8, 2019

Codecov Report

Merging #4259 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4259      +/-   ##
==========================================
+ Coverage   86.77%   86.81%   +0.03%     
==========================================
  Files         396      396              
  Lines       71550    71557       +7     
==========================================
+ Hits        62089    62121      +32     
+ Misses       9461     9436      -25

@itslarin itslarin changed the title fixed table_to_frame fun (metas lost on convertation) [FIX] table_to_frame: metas lost on convertation Dec 8, 2019
@VesnaT
Copy link
Contributor

VesnaT commented Dec 13, 2019

Thank you for the PR.

Some comments:

  • Have you thought of adding an argument to the table_to_frame i. e. include_metas=False to ensure backward compatibility?
  • Some unittests would be nice.

@janezd janezd changed the title [FIX] table_to_frame: metas lost on convertation [FIX] table_to_frame: metas lost on conversion Dec 13, 2019
@itslarin
Copy link
Contributor Author

Hi, Vesna.

Have you thought of adding an argument to the table_to_frame i. e. include_metas=False to ensure backward compatibility?

In my personal opinion this parameter can cause confusion in further usage, i believe by default table supposed to be transformed into frame without any columns lost. The user always can drop some columns explicitly if he needs. There might be some incompatibility though in case somebody rely on output frame columns number (maybe in tests). Otherwise i guess additional (in comparison with prev. results) datarfame columns shouldnt provide any harm to existing code.

@VesnaT
Copy link
Contributor

VesnaT commented Dec 16, 2019

All that is true and I agree with you, but still, the existing code should not be broken.
Nobody knows how many add-ons are using the function, all expecting dataframes without metas.
If you don't like the additional parameter, you can add it temporarily and raise a deprecation warning, to allow users to fix their code. The parameter can be removed in the next version.

@janezd
Copy link
Contributor

janezd commented Dec 16, 2019

... in the next version

Or a year. Better.

@janezd
Copy link
Contributor

janezd commented Jan 10, 2020

Decision at the meeting: @VesnaT implements what she proposed here and merges the PR.

@VesnaT VesnaT force-pushed the fix-data-pandas_compat-table_to_frame branch from 13a7bf7 to 4a9a063 Compare January 10, 2020 10:19
@VesnaT VesnaT force-pushed the fix-data-pandas_compat-table_to_frame branch from 4a9a063 to 8b2a8f5 Compare January 10, 2020 10:52
@VesnaT VesnaT merged commit ef71048 into biolab:master Jan 10, 2020
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.

4 participants