proteneer / timemachine

Differentiate all the things!
Other
138 stars 17 forks source link

get_cores raises exception when total nodes visits exceed max_visits and no cores are generated #1357

Closed badisa closed 3 months ago

badisa commented 3 months ago
proteneer commented 3 months ago

Does it make sense to consolidate max_visits and max_node_visits into just max_node_visits (or even rename max_node_visits to max_visits to avoid breaking API)? I don't see much of a use for a per-layer semantic of max_visits at this point.

badisa commented 3 months ago

Does it make sense to consolidate max_visits and max_node_visits into just max_node_visits? I don't see much of a use for a per-layer max_visits at this point.

In https://github.com/proteneer/timemachine/pull/1067 we introduced the warning. The intention was to return a core if we had hit the limit. I would be alright with making it so that the warning is thrown if len(mcs_result.all_maps) > 0 and an exception otherwise. Which would avoid the additional parameter.

badisa commented 3 months ago

@proteneer I changed the logic in https://github.com/proteneer/timemachine/pull/1357/commits/49157dc669822106daf8826ecefe779406b7ef82 to raise an exception only if no cores were found, otherwise have a warning that the core may suboptimal

mcwitt commented 3 months ago
  • The default value is set incredibly high, to preserve the old behavior
  • Raise NoMappingError exception rather than assertion to make the errors raised more uniform

I think this also changes the meaning of max_visits to accumulate over outer loop iterations (?) Should we mention that in the description and/or PR title?

badisa commented 3 months ago
  • The default value is set incredibly high, to preserve the old behavior
  • Raise NoMappingError exception rather than assertion to make the errors raised more uniform

I think this also changes the meaning of max_visits to accumulate over outer loop iterations (?) Should we mention that in the description and/or PR title?

Updated description (https://github.com/proteneer/timemachine/pull/1357#issue-2440795079) to indicate that it is the max visits across all thresholds (again think this edges, but want to get a better understanding of code before committing) and that this allows more deterministic wall clock times.