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

Ch/update readme #38

Merged
merged 16 commits into from
Oct 30, 2024
Merged

Ch/update readme #38

merged 16 commits into from
Oct 30, 2024

Conversation

cleherny
Copy link
Contributor

@cleherny cleherny commented Oct 18, 2024

  • I've updated the README to reflect the possibility of adding empty tiles or tiles including FP detection from a previous model + the multi-year aspect.
  • I've also included some changes we discussed about the assert_year function. For now, I've changed the assert to if/else to test all possible combinations relating to the use of the year. There's probably a way of simplifying this. I'll let you test your idea of adding a 'year_tile' column to the extract_xyz function.
  • Removing unnecessary lines from generate_tilesets.py.
  • I've also applied some changes to the prepara_data.py script to reflect changes and feedback from proj-sda PRs. You don't need to review it.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@GwenaelleSa GwenaelleSa left a comment

Choose a reason for hiding this comment

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

Thank you for the updates in the readme. They are good :)
I thought a little more about the function assert_year and the complicated if-else. I put my propositions in Slack.

README.md Outdated Show resolved Hide resolved
scripts/generate_tilesets.py Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

README.md Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@GwenaelleSa GwenaelleSa left a comment

Choose a reason for hiding this comment

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

The function assess_year is much better like this 👍🏻
I still proposed small modifications, as Sonar Cube still says that the cognitive complexity is too high.

@@ -147,7 +147,7 @@ def extract_xyz(aoi_tiles_gdf):

def _id_to_xyz(row):
"""
convert 'id' string to list of ints for x,y,z
Convert 'id' string to list of ints for x,y,z and t if eligeable
Copy link
Member

Choose a reason for hiding this comment

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

You said it but did not do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exact. Now it's done.

Comment on lines 208 to 214
elif str(year).isnumeric():
if 'year_tile' not in tiles_gdf.keys():
pass
else:
logger.error("Option 'year' chosen but the tile geodataframe contains a 'year' column. "
"Please delete it while producing the tile geodataframe or set the 'multi-year' option in the configuration file.")
sys.exit(1)
Copy link
Member

@GwenaelleSa GwenaelleSa Oct 28, 2024

Choose a reason for hiding this comment

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

If we want to be perfectly correct here, we should avoid negative condition as much as possible. In addition, it would allow to shorten the code.

elif str(year).isnumeric() & ('year_tile' in tiles_gdf.keys()):
     logger.error("...")
     sys.exit(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it like that trying to making explicit all the option but yes we can remove some lines.

Comment on lines 215 to 219
else:
if 'year_tile' in tiles_gdf.keys():
logger.error("Option 'year' not chosen but the tile geodataframe contains a 'year' column. "
"Please delete it while producing the tile geodtaframe or set the 'multi-year' option in the configuration file.")
sys.exit(1)

"Please delete it while producing the tile geodataframe or set the 'multi-year' option in the configuration file.")
sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, you could shorten by finishing with

elif 'year_tile' in tiles_gdf.keys():     # only possibility left for year is None or a non-authorized value
    logger.error("...")
    sys.exit(1)

This comment has been minimized.

Copy link

Passed

Analysis Details

7 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 7 Code Smells

Coverage and Duplications

  • Coverage No coverage information (0.00% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: swiss-territorial-data-lab_object-detector_AYZ4zWIzr7JdaaSXwexc

View in SonarQube

Copy link
Contributor Author

@cleherny cleherny 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 your feedback that I have taken into account.
I have one question about l139 in the main README: I should update this line as well but I have a doubt about the definition. To what refer "A" and "B" exactly?

@@ -147,7 +147,7 @@ def extract_xyz(aoi_tiles_gdf):

def _id_to_xyz(row):
"""
convert 'id' string to list of ints for x,y,z
Convert 'id' string to list of ints for x,y,z and t if eligeable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exact. Now it's done.

Comment on lines 208 to 214
elif str(year).isnumeric():
if 'year_tile' not in tiles_gdf.keys():
pass
else:
logger.error("Option 'year' chosen but the tile geodataframe contains a 'year' column. "
"Please delete it while producing the tile geodataframe or set the 'multi-year' option in the configuration file.")
sys.exit(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it like that trying to making explicit all the option but yes we can remove some lines.

Copy link
Member

@GwenaelleSa GwenaelleSa left a comment

Choose a reason for hiding this comment

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

In l. 139 of the README, A and B refer to any of the following ensemble: trn tiles, tst tiles, val tiles and oth tiles.
If A and B are not the same ensemble, their intersection is a null ensemble. It means that no tiles can be present in two of those ensembles.

I don't think you need to modify this line.

@cleherny cleherny merged commit 1f307db into master Oct 30, 2024
1 check passed
@cleherny cleherny deleted the ch/update_README branch October 30, 2024 18:07
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.

2 participants