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 #545: Add "path" arg and deprecate "raster_path" in predict_tile() #976

Merged
merged 1 commit into from
Mar 26, 2025

Conversation

Samia35-2973
Copy link
Contributor

@Samia35-2973 Samia35-2973 commented Mar 14, 2025

I sincerely apologize for the inconvenience I caused earlier. I truly appreciate your patience and guidance. If it’s okay, I have now tried to organize everything freshly with an updated version.

This PR solves the issue #545 by updating the predict_tile function to use path as an alternative to raster_path while issuing a deprecation warning when raster_path is used. I have also tried to maintain the test cases.

Here's what I modified-
In main.py:

  • Introduced path as a replacement for raster_path.
  • Issued a DeprecationWarning when raster_path is provided.
  • Updated all references to raster_path to use path.

In test_main.py: Updated the test cases to use path instead of raster_path.

In test_visualize.py: Modified test_predict_tile_and_plot to use path instead of raster_path.

I did not modify the following function in test_dataset.py

def raster_path():
    return get_data(path='OSBS_029.tif')

Please review this and let me know if this function should also be updated along with any further modifications.

@henrykironde
Copy link
Contributor

Ref Same: #975

@Samia35-2973
Copy link
Contributor Author

Sorry again for the trouble. Due to my lack of experience, I mistakenly force-pushed my branch, which unintentionally closed my previous PR. Since I wasn’t sure how to proceed, I submitted a new PR, but I now realize that might not have been the best approach. Could you kindly guide me on what the best course of action is from here?

@Abhishek-Dimri
Copy link
Contributor

Hi @Samia35-2973, no worries—this happens! Instead of creating a new PR, you can update your existing one like this:

  1. Make your changes.
  2. Amend the commit:
    git add <file>  
    git commit --amend  
    git push -f origin your-branch  

This updates the PR without adding new commits.

I faced a similar issue before, and mentor helped me to resolve it. You can see how they guided me here: PR #930.

Hope this helps! 😊

@Samia35-2973
Copy link
Contributor Author

Thanks @Abhishek-Dimri, that makes a lot of sense. Now that I opened this new PR, what do you think I should do? Should I close this new PR and try to recover the old one?

@Abhishek-Dimri
Copy link
Contributor

@Samia35-2973, it’s your choice! You can reopen the old PR and update it, or you can continue working on this new PR. I don’t think either will cause any major issues.

Could you share why you made these changes and how they’ll help improve the project?

@Samia35-2973
Copy link
Contributor Author

The changes I made are meant to align the argument names in predict_tile() and predict_image() as addressed in this issue #545. Right now, predict_tile() uses raster_path, while predict_image() uses path in the main.py file. The issue mentioned that there is no need to have more than one name for the same thing, so I’ve made predict_tile() use path instead and issued a deprecation warning for raster_path along with modifying the test cases.

@henrykironde
Copy link
Contributor

Thanks @Samia35-2973 and @Abhishek-Dimri for the contribution.

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

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

These updates look good so far. We can add a few more things.

Let's search the entire repo for predict_tile as shown here and review all instances to ensure full coverage. This will enable you update the documentation accordingly.

For these changes, please make a new commit.

@Samia35-2973
Copy link
Contributor Author

Hi @henrykironde, I have some questions regarding the changes.

  1. In docs/user_guide/examples/nest_detection.ipynb, there's a code snippet:

    # Add a path to an image to test the model on
    raster_path = ""
    predicted_raster = model.predict_tile(raster_path,
                                          return_plot=True,
                                          patch_size=300,
                                          patch_overlap=0.25)
    plt.imshow(predicted_raster)
    plt.show()

    I will change the raster_path to path for that cell. Do you want me to rename predicted_raster to predicted_image, or should I keep it as is?

  2. I am only adding the deprecated argument of predict_tile() in history.rst. Do I need to add anything else, like updating the use of path instead of raster_path in any versions?

Thanks!

@henrykironde
Copy link
Contributor

I will change the raster_path to path for that cell. Do you want me to rename predicted_raster to predicted_image, or should I keep it as is?

You can edit that code as you feel. Make sure the readers can understand the code easily.

  1. I am only adding the deprecated argument of predict_tile() in history.rst. Do I need to add anything else, like updating the use of path instead of raster_path in any versions?

To update HISTORY.rst, copy the previous template and add the new entry. I periodically collect and clean up past commits for a summary. Since we don’t have a version title yet, use a placeholder like Version x.x.x (Date: ). Feel free to include any major changes we have merged in like that of paths.

@Samia35-2973
Copy link
Contributor Author

Hi @henrykironde, I have updated all occurrences of raster_path to path argument of predict_tile() in the documentation (tutorials, user guides, examples).
I also added the deprecation notice in HISTORY.rst as discussed.
I'm happy to make any additional changes needed to ensure full coverage of this update.

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

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

Thanks @Samia35-2973 for those changes

@henrykironde henrykironde merged commit ded04cf into weecology:main Mar 26, 2025
4 checks passed
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.

3 participants