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

Update default.yaml to include the save_dir parameter. #10153

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jbdebelle
Copy link

@jbdebelle jbdebelle commented Apr 18, 2024

The goal of your commit is to address an issue encountered during validation, specifically related to the save_dir argument not utilizing the value passed to the function as intended. Instead, it utilizes an incremented path, leading to unexpected behavior.

This issue stems from the get_cfg function, where the condition if "save_dir" not in cfg: sets save_dir to None if it's not already defined in the configuration. As a result, the intended value of save_dir passed to the function is not used, leading to the undesired behavior during validation.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Introducing a customizable save directory for results in Ultralytics configurations! 🚀

📊 Key Changes

  • Added a new configuration option save_dir to specify the directory where results should be saved.

🎯 Purpose & Impact

  • Flexibility: Users can now specify a custom directory to save their results, enhancing flexibility and organization of output data. 📁
  • Improved Usability: Makes it easier for users to manage and locate their output files, streamlining workflows and analysis. 🛠️
  • Potential Impact: This change could significantly improve user experience by providing more control over where results are stored, potentially leading to better data management and analysis practices. 🌈

The goal of your commit is to address an issue encountered during validation, specifically related to the save_dir argument not utilizing the value passed to the function as intended. Instead, it utilizes an incremented path, leading to unexpected behavior.

This issue stems from the get_cfg function, where the condition if "save_dir" not in cfg: sets save_dir to None if it's not already defined in the configuration. As a result, the intended value of save_dir passed to the function is not used, leading to the undesired behavior during validation.
Copy link

github-actions bot commented Apr 18, 2024

CLA Assistant Lite bot All Contributors have signed the CLA. ✅

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hello @jbdebelle, thank you for submitting an Ultralytics YOLOv8 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with ultralytics/ultralytics main branch. If your PR is behind you can update your code by clicking the 'Update branch' button or by running git pull and git merge main locally.
  • ✅ Verify all YOLOv8 Continuous Integration (CI) checks are passing.
  • ✅ Update YOLOv8 Docs for any new or updated features.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." — Bruce Lee

See our Contributing Guide for details and let us know if you have any questions!

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.93%. Comparing base (fcfc44e) to head (4cd2df9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10153      +/-   ##
==========================================
- Coverage   73.46%   69.93%   -3.53%     
==========================================
  Files         122      122              
  Lines       15622    15622              
==========================================
- Hits        11476    10926     -550     
- Misses       4146     4696     +550     
Flag Coverage Δ
Benchmarks 35.35% <ø> (-0.20%) ⬇️
GPU 37.28% <ø> (-0.02%) ⬇️
Tests 66.13% <ø> (-3.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jbdebelle
Copy link
Author

I have read the CLA Document and I sign the CLA

@jbdebelle
Copy link
Author

recheck

@glenn-jocher
Copy link
Member

@jbdebelle certainly! 😊 It looks like you're requesting a re-evaluation of a particular aspect or issue. To assist you effectively, could you please provide more specific details about what you need rechecked? If it's pertaining to a potential bug or an update in code or documentation, a brief code snippet or direct link to the concerned part would be immensely helpful!

@jbdebelle
Copy link
Author

args = dict( model=model, imgsz=size, max_det=max_det, data=data, split=split, conf=conf, iou=iou, task=task, save_dir=save_dir, ) return YOLO(model).val(**args)

In the code above is I specify a custom save_dir path, it will not be taken into account. 
By adding save_dir in the default.yaml file, this mitigates this issue. 

@glenn-jocher
Copy link
Member

Thanks for pointing this out! 😊

Indeed, including save_dir in your default.yaml and passing it through your code as shown, should direct your results to the specified directory. Just ensure the save_dir is defined in your default.yaml like so:

save_dir: 'path/to/your/directory'

And the snippet you've provided should work as intended. If it doesn't, making sure that default.yaml is correctly loaded before running your validation will be key.

Keep us posted if you encounter any other issues! 🛠️

@jbdebelle
Copy link
Author

Then shouldn't my PR be merged into the main? This means that the current implementation doesn't support the use of the save_dir argument. My suggested fix will resolve this issue.

@glenn-jocher
Copy link
Member

Thanks for bringing this to our attention! 🌟 You're right; your PR addresses the save_dir argument not being effectively used as intended. This is an important fix, and we appreciate your effort to improve the functionality. We'll review the proposed changes closely and proceed with merging if everything aligns well with our project goals and standards. Please bear with us as we go through this process.

Keep contributing! Your input helps make our project better for everyone. 👍

@n1mmy
Copy link

n1mmy commented May 5, 2024

+1. I encountered the same issue of save_dir not working. Modifying default.yaml worked for me to fix the issue. Thanks for this!

@glenn-jocher
Copy link
Member

Hi there! 🎉 I'm glad to hear that modifying default.yaml resolved the save_dir issue for you. Thanks for sharing your successful workaround—it's really helpful for the community. If anyone else encounters this, they can apply the same fix by adding:

save_dir: 'desired/path/for/output'

to their default.yaml. Happy coding and keep the feedback coming! 👍

@luca-depe
Copy link

Hi @glenn-jocher! Will this be merged soon?

Copy link

@luca-depe luca-depe left a comment

Choose a reason for hiding this comment

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

Nice catch!

@glenn-jocher
Copy link
Member

@luca-depe hello!

Thanks for reaching out. We're currently reviewing the changes to ensure everything aligns perfectly with our standards. If all goes well, it should be merged shortly. We appreciate your patience and enthusiasm! 🚀

Stay tuned!

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.

None yet

4 participants