Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Added lowercase split value and passing tests#42 #67

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Added lowercase split value and passing tests#42 #67

wants to merge 12 commits into from

Conversation

Samad999777
Copy link

This pull request takes care of the lower case support issue and enables the users to follow the normal convention as mentioned in #42

This is mainly a cosmetic change and the dataset labels such as 'TRAIN' ,'TEST', 'VALIDATE' are not compatible as lower cases versions. It does not affect the broader scope of the project. however it may require documentation changes.

Fixes #42

Cosmetic change. as explained in summary above.

Please delete options that are not relevant.

  • [ Y ] New feature (non-breaking change which adds functionality)
  • [ Y ] This change requires a documentation update

Checklist

  • [Y] My code adheres to the Google Python Style Guide
  • [Y] I ran make test and all tests pass
  • [Y] I ran the tool and verified the change works
  • [Y] I have adequately commented my code, particularly in hard-to-understand areas
  • [Y] My changes generate no new warnings
  • [Y] I have corrected any misspellings in my code
  • [Y] (For feature) I made sure that I'm merging to the dev branch

cfezequiel and others added 6 commits November 4, 2020 10:05
@cfezequiel @mbernico @lc0
Release/2.0 (google#56)
f4650ca 
* Update check_tfrecords to use new dataset load function.

* Add tfrecord_dir to create_tfrecords output.

* Restructure test image directory to match expected format.

* Feature/dataclass (google#44)

* Added data classes for types.

* Checking in progress.

* Checking in more changes.

* Converted types to classes and refactored schema into OO pattern.

* Changed OrderedDict import to support py3.6.

* Changed OrderedDict import to support py3.6.

* Updated setup.py for version.

* fixing setup.py

* Patched requirements and setup.

* Addressed comments in code review.

* Addressed code comments round 2.

* refactored IMAGE_CSV_SCHEMA.

* Merged check_test.py from dev

Co-authored-by: Carlos Ezequiel <cezequiel@google.com>

* Feature/structured data tutorial (google#45)

* Converted types to classes and refactored schema into OO pattern.

* Add tutorial on structured data conversion.

This changes types.FloatInput to use tf.float32 for its feature_spec
attribute to address potential incompatibility with using tf.float64
type in TensorFlow Transform.

Co-authored-by: Mike Bernico <mikebernico@google.com>

* Update structured data tutorial to use  output dir.

* Clarify need for proper header when using create_tfrecords. Fixes google#47.

* Clean up README and update image directory notebook.

* Feature/test image dir (google#49)

* Restructure test image directory to match expected format.

* Clean up README and update image directory notebook.

* Fix minor issues

* Add an explicit error message for missing train split

* Configure automated tests for Jupyter notebooks.

* Add convert_and_load function.

Also refactor create_tfrecords to convert.

* Refactor check and common modules to utils.

* Add test targets for py files and notebooks.

* Feature/convert and load (google#55)

* Add convert_and_load function.

Also refactor create_tfrecords to convert.

* Refactor check and common modules to utils.

* Add test targets for py files and notebooks.

* Update version in setup.py and release notes.

* Fix issues with GCS path parsing.

Co-authored-by: Mike Bernico <mikebernico@google.com>
Co-authored-by: Sergii Khomenko <khomenko@brainscode.com>
@cfezequiel
Merge branch 'release/2.0'
20617ff
@Samad999777
added lowercase split value google#42
10ad553
@Samad999777
added lowercase split value google#42
5f26369
@Samad999777
Updated the 'data.csv' labels & successfully ran tests on windows
cc5d255
@Samad999777
Updated the 'data.csv' labels & successfully ran tests on windows
f880215
@google-cla google-cla bot added the cla: yes label Jun 29, 2021
cfezequiel
cfezequiel suggested changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@cfezequiel cfezequiel left a comment

Choose a reason for hiding this comment

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

Thanks for making a new PR with the changes. I added some comments.
Please make sure to pass all the tests, including the ones for Jupyter notebooks.

tfrecorder/types.py Show resolved Hide resolved
tfrecorder/utils_test.py Outdated Show resolved Hide resolved
@Samad999777
Copy link
Author

@cfezequiel kindly review the new commits

cfezequiel
cfezequiel reviewed Jul 13, 2021
View reviewed changes
Copy link
Contributor

@cfezequiel cfezequiel left a comment

Choose a reason for hiding this comment

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

Thanks for updating the notebooks. I added a few comments.

samples/Basic-TFRecorder-Usage.ipynb Outdated Show resolved Hide resolved
samples/Basic-TFRecorder-Usage.ipynb Outdated Show resolved Hide resolved
@Samad999777
Copy link
Author

@cfezequiel Kindly review the changes

@Samad999777
Copy link
Author

@cfezequiel Did you review the changes ?

cfezequiel
cfezequiel suggested changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@cfezequiel cfezequiel left a comment

Choose a reason for hiding this comment

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

@Samad999777 sorry for the long delay in response. Just got back to reviewing this PR and also testing your changes. I added one suggestion. Once you resolve it I think this should be good to check in.

tfrecorder/test_data/data.csv Outdated Show resolved Hide resolved
@Samad999777
Copy link
Author

@cfezequiel alright,
I will make the changes and hopefully we can publish these last changes as well.

@Samad999777 @cfezequiel
Update tfrecorder/test_data/data.csv
9b51ddd 
Co-authored-by: Carlos Ezequiel <cafezequiel@gmail.com>
@google-cla google-cla bot added cla: no and removed cla: yes labels Jan 14, 2022
@Samad999777
Copy link
Author

@cfezequiel can you kindly review again and approve ?

@cfezequiel
Copy link
Contributor

@Samad999777 looks like there is a conflict with one of the files. Could you please check?

@Samad999777
Copy link
Author

Samad999777 commented Jan 14, 2022 •
edited

@cfezequiel sure, i'll review

@Samad999777
Copy link
Author

@cfezequiel I've resolved the conflict. Can you review ?

@cfezequiel
Copy link
Contributor

@Samad999777 got it. There seems to be a cla issue but it looks like something I need to resolve on my end.

@Samad999777
Copy link
Author

@cfezequiel yes, i think you need to re-submit your CLA.

@Samad999777
Copy link
Author

@cfezequiel can you kindly take care of the CLA so these changes can be pushed?

@cfezequiel
Copy link
Contributor

@Samad999777 I may need to update the commit in your PR and push the change to your branch to address the CLA issue (basically remove the unregistered email address there). Are you ok with that?

@Samad999777
Copy link
Author

@cfezequiel As long as i get to be the contributer of this pull request in the project. I am very much okay with it.

@cfezequiel
Copy link
Contributor

@Samad999777 are you able to revert this commit: "Merge branch 'dev' into main" and do a rebase against the dev branch instead? That way, your commits come out on top in the commit history.

@Samad999777
Revert "Merge branch 'dev' into main"
ecc95d8 
This reverts commit 3797da3, reversing
changes made to 9b51ddd.
@google-cla google-cla bot added cla: yes and removed cla: no labels Jan 25, 2022
@Samad999777
Copy link
Author

@cfezequiel I have reverted the commit: "Merge branch 'dev' into main". However when i attempted to rebase it says that it is already upto date [see image below]. Am i doing this right ?
image

@cfezequiel
Copy link
Contributor

@Samad999777 Thanks for the change. Have you tried rebasing the main branch in your fork with this repo's dev branch? PRs should be made off of the dev branch unless it's a hotfix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cfezequiel cfezequiel Awaiting requested review from cfezequiel

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Samad999777 @cfezequiel