Skip to content

Bug fixs#2

Open
AbdelazizBouzidi wants to merge 2 commits intoClementPinard:masterfrom
AbdelazizBouzidi:master
Open

Bug fixs#2
AbdelazizBouzidi wants to merge 2 commits intoClementPinard:masterfrom
AbdelazizBouzidi:master

Conversation

@AbdelazizBouzidi
Copy link
Copy Markdown

No description provided.

@ClementPinard
Copy link
Copy Markdown
Owner

/quack review

@ghost
Copy link
Copy Markdown

ghost commented Jun 15, 2023

Oops, that one is on us 🙃 We'll investigate shortly!

apologies

@frgfm
Copy link
Copy Markdown

frgfm commented Jun 15, 2023

/quack review

Copy link
Copy Markdown

@ghost ghost 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 the PR 🙏

Other sections
  • [invalid-eof] detected at generate_sky_masks.py:L106
  • [print-statement] detected at main_pipeline_no_lidar.py:L77
  • For a better experience, it's recommended to check the review comments in the tab Files Changed.


    Feedback is a gift! Quack will adjust your review style when you add reactions to a review comment

    Comment thread generate_sky_masks.py
    file_exts = ['jpg', 'JPG']

    process_folder(args.img_dir, args.colmap_img_root, args.mask_root, file_exts, True, args.batchsize)

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    The EOF should be a single blank line

    Pattern explanation 👈 Adding a blank line as End of File (EOF) is a convention for file processing to mark the end of a character sequence. For more details, feel free to check this resource.

    Suggestion

    Perhaps we could handle this like so:

    Suggested change

    Quack feedback loop 👍👎 This comment is about [invalid-eof]. Add the following reactions on this comment to let us know if:
    - 👍 that comment was on point
    - 👀 that doesn't seem right
    - 👎 this isn't important for you right now
    - 😕 that explanation wasn't clear

    Comment thread main_pipeline_no_lidar.py
    if i not in args.skip_step:
    print_step(i, "First thorough photogrammetry")
    env["thorough_recon"].makedirs_p()
    print(env["video_frame_list_thorough"])
    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    Let's avoid print calls in production-ready code

    Pattern explanation 👈 print calls can be useful for developping and debugging. But moving to a production-ready system, users can select the logging level they want to display but prints don't offer that flexibility. For more details, feel free to check this resource.

    Code example

    Here is an example of how this is typically handled:

    # print("The payload is empty!")
    import logging
    
    logging.warning("The payload is empty!")

    Quack feedback loop 👍👎 This comment is about [print-statement]. Add the following reactions on this comment to let us know if:
    - 👍 that comment was on point
    - 👀 that doesn't seem right
    - 👎 this isn't important for you right now
    - 😕 that explanation wasn't clear

    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