jerbaroo / bridge-sim

Python library for concrete slab bridge simulation.
MIT License
1 stars 0 forks source link

Inconsistent input naming #154

Closed rozsasarpi closed 4 years ago

rozsasarpi commented 4 years ago

https://github.com/barischrooneyj/bridge-sim/blob/53bc6aa1ef23cf2d3c76a0914347055598ffcb97/public/bridge_sim/model/__init__.py#L20

It is strange that you prescribe the unit of the load intensity but not that of the geometrical dimensions. Is there a reason for this decision?

jerbaroo commented 4 years ago

Reason was: names are short, further description in comments. Though really I am not a fan of short naming for the sake of it, a good editor will have some form of auto-completion.

Consistent options:

Aside: I am slowly moving things into the public folder. This should be the "nice" API, after things got a bit messy. During this migration I am prefacing things imported from the private "messy" part with lib. Eventually I hope to bring everything from private to public.

Second aside: PointLoad should not need to take a Config as first argument, this will be removed at some point during this migration to a nicer API, legacy/silly design decision.

jerbaroo commented 4 years ago

A: Another idea is that units are written down only as a type annotation. I like this solution. To me types in code seem analogous to units in Physics. A good editor e.g. PyCharm should show the type on hover.

Definition: PointLoad(x: Metre, z: Metre, load: KiloNewton)

Usage: PointLoad(x=1, z=1, load=1)

B: I think I will make sure everything is SI, so kN -> N.

rozsasarpi commented 4 years ago

I would not suggest to do A, it seems to be too error prone. Unless you can check if the user truly provided the input in the units you have in the type annotation.

I'm torn between two main approaches:

  1. no indication of units, it is the user's responsibility to ensure consistency; this is the typical decision of finite element software developers (except those that are meant for design offices such as Axis where the unit system is built in and the user can select different variants). This approach would give the most freedom but would put some burden on the user.

  2. Indicating units (either in variable naming or using a units package). This would remove the burden of keeping the units in mind and consistent but would restrict the freedom of the user, e.g. Americans with imperial units (though I care little about their silly system). Given the scope of this package a prescribed unit system would not necessarily be too restrictive. If sticking to SI will not create overly tiny or huge numbers than that would be also a good decision.

I lean towards 2. but I'm not fully convinced.

jerbaroo commented 4 years ago

For the time being, to have some consistency, I will do 1. (since it's a small change and will provide consistency with naming, and with other FE packages).

However I really would like to add support for some kind of unit checking in the future, but I don't want to rush it. I prefer not to do so with variable naming, but with some (ideally optional) system that will check the units are correct, either via static analysis or at run-time. I noticed the pint units package supports different unit systems.

I think any future discussion on unit checking should move to #160 .

rozsasarpi commented 4 years ago

Ok, sounds good. If you go with 1. then by default you should not have units on the plots either.