search

czbiohub-sf / shrimPy

shrimPy: Smart High-throughput Robust Imaging & Measurement in Python
BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

Validate acquisition settings #73

Closed ieivanov closed 1 year ago

ieivanov commented 1 year ago

Fixed #40

talonchandler commented 1 year ago

Nice find! Thanks @ieivanov.

I think I can pick up from here and also forbid extra top-level parameters.

ieivanov commented 1 year ago

That do you mean by extra top level parameters? The acquisition is designed such that if the user does not provide channel settings they would be left blank. There is a danger of the user misspelling lf_channel_settings, I guess. But I think at this point we don't need to be that thorough.

On Tue, Aug 1, 2023, 6:26 PM Talon Chandler @.***> wrote:

Nice find! Thanks @ieivanov https://github.com/ieivanov.

I think I can pick up from here and also forbid extra top-level parameters.

— Reply to this email directly, view it on GitHub https://github.com/czbiohub-sf/mantis/pull/73#issuecomment-1661325282, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3BXKCYJCT4SQNIWWVKJF3XTGUFFANCNFSM6AAAAAA3AQX3CI . You are receiving this because you were mentioned.Message ID: @.***>

talonchandler commented 1 year ago

Sounds good...I skipped the top level parameters. I couldn't see a path that didn't break backwards compatibility.

I did a quick skim through and added some slightly tighter types. Otherwise, LGTM!

ieivanov commented 1 year ago

@talonchandler can you please apply this config to the analysis settings as well and add tests? I'll take a closer look at the acquisition settings tests

talonchandler commented 1 year ago

@ieivanov good call. Fixed!

ieivanov commented 1 year ago

Ready to merge, I think, can I get an approval?