cauldron / activity-browser

GUI for Brightway
GNU Lesser General Public License v3.0
0 stars 3 forks source link

Feat/contribution tree #43

Closed will7200 closed 1 month ago

will7200 commented 2 months ago

Checklist

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 10548903660

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
activity_browser/ui/web/base.py 1 2 50.0%
activity_browser/application.py 1 3 33.33%
activity_browser/ui/web/tree_navigator.py 52 264 19.7%
<!-- Total: 56 271 20.66% -->
Files with Coverage Reduction New Missed Lines %
activity_browser/application.py 1 73.53%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 10507369358: -0.6%
Covered Lines: 8159
Relevant Lines: 15061

💛 - Coveralls
cmutel commented 2 months ago

Thanks @will7200 .Can we have a call to talk about this? Shouldn't take too long. Maybe in the next 6 hours?

will7200 commented 2 months ago

@cmutel For the tag types, since they are aimed at being categorical data types, what data types do we want to support? The tags support json values so that consist of numbers, strings, arrays, and objects. Currently I implemented support for:

So we can drop the number ones (int and float). Do we need to support others ones? How much benefit would supporting lists and objects?

cmutel commented 2 months ago

So we can drop the number ones (int and float). Do we need to support others ones? How much benefit would supporting lists and objects?

A good question - probably the schema is too loose here. I can see a good argument for int, in some numbered classification scheme, but am struggling to see how float would make sense (especially considering underlying floating point mechanics). I also agree that datetimes smell a bit fishy, but if converted to ISO 8601 then that seems OK.

Definitely not objects including lists or any other nonsense. Special cases aren't special enough.

will7200 commented 1 month ago

@cmutel Ready for review will require https://github.com/brightway-lca/bw_graph_tools/pull/20 to be merged for tests to pass