Skip to content

Conversation

@ovalle15
Copy link
Contributor

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -38,34 +38,52 @@
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the unused imports please.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done !

@@ -38,34 +38,52 @@
},
Copy link
Contributor

@msokoloff1 msokoloff1 Jan 3, 2023

Choose a reason for hiding this comment

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

One thing that is lost in these updates is the tools for geospatial transformations. Here you have hard coded all of the object coordinates. But almost every ML engineer has to either start with a model prediction and project into wgs84 coordinates or start with labels and project into pixels. imo these tools are super cool and there is no great open source alternative. Even if the main point of the notebook isn't to do the projection we should highlight it.

Reply via ReviewNB

Copy link
Contributor Author

@ovalle15 ovalle15 Jan 3, 2023

Choose a reason for hiding this comment

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

I see, this will require a bit more work. Can I suggest demonstrating the geospatial transformations on a different notebook ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msokoloff1 do you think creating a new notebook that only focuses on geospatial transformations could work? Otherwise, the current notebook would be too complex, in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. ! Included a polygon transformation on Step 1 below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to step 5

@@ -38,34 +38,52 @@
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Line #22.    

I don't think this is necessary any more. I think it uploads in-line. Please double check if this is needed.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed !

@ovalle15 ovalle15 changed the title New NDJSON format New NDJSON format - tile imagery Jan 3, 2023
1. Removed unused imports
2. Removed signing function 
3. Updated file path
Included CV2 and PIL python libraries to generate the polygon annotation
@@ -20,12 +20,12 @@
},
Copy link
Contributor

@msokoloff1 msokoloff1 Jan 31, 2023

Choose a reason for hiding this comment

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

Line #8.    import gdal

You aren't directly referencing this. I wouldn't import it.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -20,12 +20,12 @@
},
Copy link
Contributor

@msokoloff1 msokoloff1 Jan 31, 2023

Choose a reason for hiding this comment

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

from labelbox.schema.ontology import OntologyBuilder, Tool, Classification, Option
from labelbox import Client, LabelingFrontend, LabelImport, MALPredictionImport

These are all available in the top level namespace.



Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -20,12 +20,12 @@
},
Copy link
Contributor

@msokoloff1 msokoloff1 Jan 31, 2023

Choose a reason for hiding this comment

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

Line #4.    import cv2

These are duplicates. I would just add all imports at the top.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Modified order of the cell with polygon 2 annotation
@ovalle15 ovalle15 merged commit 80f07e3 into develop Jan 31, 2023
@ovalle15 ovalle15 deleted the ovalle15-patch-5 branch January 31, 2023 20:10
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