Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds unit conversion #20

Closed
wants to merge 51 commits into from
Closed

Conversation

kdschlosser
Copy link
Contributor

run the unit_converter.py script to see an example.

This script can be added onto, it does contain most of the typical unit conversions.

run the unit_converter.py script to see an example.
@coveralls
Copy link

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling 7ba9cf2 on kdschlosser:unit_conversion into 824a871 on WoLpH:develop.

@wolph
Copy link
Owner

wolph commented Jun 15, 2021

Wow, that's a pretty extensive bit of code. Great work :)

In workings it seems somewhat similar to the internals of my Alfred Converter package: https://github.com/WoLpH/alfred-converter/
The big difference is that I was lazy with that one and have imported the units from an external file ;)

I do have a few questions though:

  1. It looks like al of the units are inheriting int, but I assume that many calculations return a float instead. Is that intentional?
  2. Would it be a good idea to change the _values or _IntWrapper into a namedtuple? It would function mostly the same but it might be clearer what all the numbers mean

I should note that I'm really busy so I'm not yet sure when I can finish merging and releasing this. I haven't forgotten about your other pull request either but haven't gotten to it yet :(

@kdschlosser
Copy link
Contributor Author

Oh I know that this is going to need to be cleaned up and optimized.

To answer your question about the int/float thing.
The returned value all depends on the input value and also the conversion. Even tho an input value may be an int the return value may be a float depending on whether or not a float calculation was done. Maybe I should have it just return a float all the time no matter what the input value is. Would make it easier so a person wouldn't have to do type checking at all when they want to use the returned value.

Would it be a good idea to change the _values or _IntWrapper into a namedtuple? It would function mostly the same but it might be clearer what all the numbers mean

The constants need to be there in order to identify what type of conversion is to be done. In the eyes of the user the constants are an int and that is the only thing they need to know about. The values and label contained within the constant are for internal use only. The reason for the property is so that a user does not change what the values are if they discover them there. If you notice inside of the property I return a copy of the list. so in the event that they do change the values it is not going to effect the running of the unit converter.

I could have hard coded those values instead if doing what I did. but for code readability I set the values into the constant class. I did this so it is easier to identify what UOM belongs to what data type.

I wanted to eliminate the need for using a lookup table to store conversion factors. That kind of a table can get pretty large in size so by using the SI model idea and applying it to other types I was able to remove the majority of the hard coded factors.

@kdschlosser
Copy link
Contributor Author

Ya know I really do hate the crap the developers of Python are doing. They are doing things that is removing the flexibility of Python which was the best thing about it.

So I am trying to make a wrapper class around int and float if the value gets converted to a string it will include the unit. Now that in and of it's self is not a problem. I wanted to carry that unit if they converted the returned value to an int. Nope can't do that. They will only allow the returning of and instance of the class int from __int__ it cannot be a subclass of int. ok that sucks. It is the same deal with __float__. So now I decided I would point the magic methods to functions after the instance has been created. NOPE can't do that either, says the magic methods are read only.

Those bone heads never think about the people that use Python, they come up with these trivial things to change for absolutely no reason. They it with ctypes and passing a structure to a function that was not a pointer. It completely trashed using COM objects in Windows from Python. And now this crap with only allowing a return of an instance of int or float and not even a subclass of either of them. Then why give the ability to override those 2 magic methods at all.

@wolph
Copy link
Owner

wolph commented Jun 16, 2021

Yeah... there was a huge discussion about this some time ago: https://mail.python.org/pipermail/python-dev/2013-April/125042.html / https://bugs.python.org/issue26984

Long story short, the idea was that because int(...) is assumed to be a cast to int, it should always return an int.

Perhaps the more important question should be, if you're not looking for an int, why are you calling int()?
You can specify how calculations and such work using methods like __add__, __div__, __mul__, etc... And you're under no obligation to return an int/float from those

@kdschlosser
Copy link
Contributor Author

well here is a use case. I could understand if returning a instance that is not an instance of a subclass of int then block it. But for crying out loud to block even subclasses of an int is just bonkers.

@kdschlosser
Copy link
Contributor Author

what's really not right about it is the fact that this is only on int and float. I tested it with str and the __str__ method returning a subclass of str from __str__ and it works without a hitch.

I can't even add a variable to an instance of int because int has no __dict__ setattr doesn't work either. I was hoping to add a unit variable as a backup but NOPE!

They are making code changes that fix no bug and have never had any kind of an issue in the first place. I don't understand why they would change it when there is no reason to do so.

@kdschlosser
Copy link
Contributor Author

It's funny because person after person after person are for returning an int or a subclass of int. Not 1 person mentioned the use of int(float()).real or float(int()).real for accessing an actual int or an actual float instead of the possibility of a subclass.

@kdschlosser
Copy link
Contributor Author

and the statement you linked to has this statement

int() and operator.index() are both type coercion calls to produce true
Python integers - they will never return a subclass, and this is both
deliberate and consistent with all the other builtin types that accept an
instance of themselves as input to the constructor.

This is a false statement 100%

class TestClass(str):

    def __str__(self):
        return TestClass('This is a test')


test = TestClass()
print(type(str(test)))

and the output is

<class '__main__.TestClass'>

funny how that is. someone dimwit got a wild hair across their ass about int() and float() possibly having subclasses returned for some crazy reason and it gets called a bug when it obviously is not

@wolph
Copy link
Owner

wolph commented Jun 16, 2021

I agree. It's one of the things that I really dislike about Python.

My guess is that it's related to compatibility with the C base however. Making these parameters changeable could result in a crashing interpreter if the C code doesn't handle it properly.

@kdschlosser
Copy link
Contributor Author

Well it works just fine in python 2.7 all the way up to 3.4 I believe.

There shouldn't be an issue with returning a subclass is int because the person running the code either wrote it and already knows about the subclass. Or the person installed a library and if they read the documentation or look at the code for the library they will know that it returns a subclass of an int. This all boils down to not running code that you are unfamiliar with.

and again there is still the real property that can be used to obtain a non subclassed version of the int or float. with what they did then real no longer serves a purpose and can be removed because wrapping a float in float() or an int in int() is going to return the exact same thing as real

@wolph
Copy link
Owner

wolph commented Jun 18, 2021

Well it works just fine in python 2.7 all the way up to 3.4 I believe.

It probably works in most cases, but it's likely to cause issues in some scenarios from what I've read. Memory corruption is not fun to deal with ;)

There shouldn't be an issue with returning a subclass is int because the person running the code either wrote it and already knows about the subclass. Or the person installed a library and if they read the documentation or look at the code for the library they will know that it returns a subclass of an int. This all boils down to not running code that you are unfamiliar with.

The problem is with the underlying C code, if that expects a variable to be 8 bytes (int64) and it's an object instead that takes many more bytes, that could be very problematic. While I expect most code to be compatible, there is no guarantee so that could cause problems.

@kdschlosser
Copy link
Contributor Author

I guess it really doesn't matter because a user has to pass in the unit they want to convert to so they actually have the unit they can attach to a string version of the return value. It's just that I need to make it so that the return value is always a float and the user can do whatever it is they want with the returned value after that.

The only other thing I am going to change is using decimal.Decimal for the floating point math. I will still have it return a float tho.

Changed to using `decimal.Decimal` for better floating point accuracy.

Removes having to pass a "unit type" constant

Main entry point now always returns a float

General code cleanup.
@lgtm-com
Copy link

lgtm-com bot commented Jun 19, 2021

This pull request introduces 1 alert when merging 06a598b into 824a871 - view on LGTM.com

new alerts:

  • 1 for Module is imported more than once

@kdschlosser
Copy link
Contributor Author

I just made a commit that has several changes to it.

The unit type constants are no longer public and do not need to be massed to the main entry point.
I changed over to using decimal.Decimal so floating point errors are handled properly.
I changed the example at the end of the file so it shows conversion to a unit and then back again.
I also did some code cleanup.

I have done some pretty crazy unit conversions when dealing with Internal Combustion Engines. an example is converting from grams a second of airflow or cubic feet a minute of airflow to horse power and torque with optionally taking into account barometric pressure, humidity, air temperature and also the gravitational forces due to location on the planet. That last one is over the top I know but I figured what the hell. It's all about calculating the number of oxygen molecules being used for combustion and gravitational forces do effect how oxygen dense a volume of air is.

I have a slew of them I have written up dealing with all kinds of things related to the automotive world. Any interest in those at all? I know they are very specific in nature but they are unit conversions.

@lgtm-com
Copy link

lgtm-com bot commented Nov 1, 2021

This pull request introduces 3 alerts when merging 5feeea2 into a5b8dad - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call
  • 1 for Module is imported more than once

@kdschlosser
Copy link
Contributor Author

not sure why the conversion of all of the double quotes to single quotes. Well I can understand everything except for the docstrings.

https://www.python.org/dev/peps/pep-0257/

@wolph
Copy link
Owner

wolph commented Nov 1, 2021

It's simple, consistency: https://www.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds

The rest of the codebase uses single quotes as well :)

@kdschlosser
Copy link
Contributor Author

I know I saw that all of the docstrings used single quotes except for converts.remap
I just hate seeing the squiggly lines in my IDE.

Don't add this exact code for the unit converter into your library. I hate it as it is hard to follow what in the hell is going on. I need to clean it up a bunch . I was waiting until you had the time to lend a hand/provide input with it. I also want to add to it returning a normalized uom for the returned value. So if you tell the converter to convert from "sq m" to "sq ft" when the value is returned it will return a uom of u'ft²'. This is a nice thing to have available if you are presenting the data to a user. hard coding the unicode characters is kind of a pain to have to do. much easier to pass "sq ft" and have the much nicer looking u'ft²' returned for use when presenting the data to the user.

I also want to clean up the code so it is easier to understand what is going on and that will make it easier to maintain/fix.

General code cleanup and thinning.
General code cleanup and thinning.
…rsion

# Conflicts:
#	python_utils/unit_converter.py
General code cleanup and thinning.
@kdschlosser
Copy link
Contributor Author

sorry about the mess with the commits. because you edited my branch in this PR I didn't even think to update my local copy before making changes. It made a mess on my end locally that I had to correct.

@lgtm-com
Copy link

lgtm-com bot commented Nov 1, 2021

This pull request introduces 2 alerts when merging da27386 into a5b8dad - view on LGTM.com

new alerts:

  • 2 for Unused named argument in formatting call

@wolph
Copy link
Owner

wolph commented Nov 2, 2021

Oh... that's weird. I tought I modified my own copy of it. I didn't know github would give me write access to your version... that's weird

In any case, single quotes is fully PEP8 compliant so your editor shouldn't be complaining about it :P

With regards to the sq vs ², I think some kind of tokenization is in order for that use-case but there are many ways of implementing that.

I think dataclasses could be nice to store the units and their possible units. And a parent dataclass for the type of measurement (i.e. temperature).
You might also need a way to specify combinations of units though... since some units are combinations of other units but that quickly becomes complicated.

@lgtm-com
Copy link

lgtm-com bot commented Nov 28, 2021

This pull request introduces 1 alert when merging 4b11ee3 into 82b48d0 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass

@wolph
Copy link
Owner

wolph commented Nov 29, 2021

Very nice! This was exactly along the lines of what I was planning: 12963f8

In addition I'm planning to update the code to use Python 3.5+ type hints. And add the units to the units object so you can easily autocomplete and jump to the definition.

This is another big update to the program.

The quantity system is designed in a manner that allows a user to add additional quantities and that gets done by subclassing the `Quantities` class. There is a metaclass attached to `Quantities` that handles injecting the newly created class. An instance of the class does not need to be made.

It is important to use CaMeL case for the class name. The class name gets parsed and a space is added before every uppercase letter (except  the first) and that gets set to the class attribute `name`. `name` does not have to be set manually unless you want the name to to be different then the class name.

The docstring gets turned into a single line description that is accessible from the description class attribute.

You do not need to set the description attribute unless you want the description to be different then the docstring.

The symbol attribute is optional. It is a nicety that just gives more information is wanted.  At this time it does not effect hot the program runs at all.

The unit attribute is mandatory, this is what the program uses to determine what units are compatible. Each quantity in the SI system typically has a unit attached to it. This unit is what units gets converted to and converted from. It is what all conversion factors for a specific quantity are set for.

How the program check for compatible units is every unit in the SI system breaks down into one or more of 7 base units. by comparing the "SI equivalent" or "SI expression" for a quantity to the same for another quantity if they match then the quantities are compatible and conversions can be done across them.

Here is an example of how to correctly add a quantity to the program

```python

from python_utils.unit_converter.quantity import Quantities

class Crackle(Quantities):
    """
    Change of jounce per unit time: the fifth time derivative of position
    """
    symbol = 'c'
    unit = units.m / units.s(exponent=5)
```

The quantities in the SI system are wild as there are only a small number of them that are in the SI standard. Most of the SI quantities are created based on use of the SI system. Here are some examples.

Physics
Chemistry
Mechanical
Radiometry
Optics
Molar
Electromagnetic
Photometric

Each of those fields has SI quantities that relate to different unit groupings. I want to note that there are overlaps between the different fields and also with the quantities written into the SI standard. They may share the same name and they may not. If the name is not shared but you would like to have the correct name attached to the quantity then subclass the quantity you want  to create a new name for. You do not have to specify the unit when doing this if the unit is the same. All other attributes function the same way.

Now remember an instance of the new quantity does not need to be created for this to work    you simply hve to define the quantity class and either subclass `Quantities` or another quantity class.

I have done a large amount of trial and error in order to get the best possible speed while keeping the memory footprint as small as possible. In the  example file at the end I have the system enumerate all of the core units by way of iterating over all available quantities and the compatible units for those quantities.

There are 514 core units and 179 core quantities I have added so far. I still have to add the units and quantities related to Optics and any other fields of study that use the SI system. I have not been able to locate a list fo field of study that use the SI system so locating them has been a matter of stumbling across them on accident.
@kdschlosser
Copy link
Contributor Author

Happy reading!!

And add the units to the units object so you can easily autocomplete and jump to the definition.

You don't actually have to add them to the dynamic module class. Because of the smoke and mirrors of how that whole thing works an IDE is not going to be able to trace that way through the program so what it sees is whatever is specified at the module level.

Now.. That being said. there is a bug in both PyCharm and also in Sphinx. PyCharm does not use the .. py:data:: :type: directive for determining a "constants" data type. Sphinx does not use the current context of where the docstring is located in order to link to the object set in the directive unless the entire module path is supplied. I cannot import Unit and set the :type: to Unit. The :type: option needs to be set to python_utils.unit_converter.unit.Unit explicitly. Doing that produces a crappy output in the documentation. I reported the bug to the developers of Sphinx and it has already been fixed and the fix will be included in the next release of Sphinx.

sphinx-doc/sphinx#9899

I have not yet created a bug report with JetBrains yet. Not that I would actually think they would fix it any time soon anyhow.

The really shitty thing is Sphinx outputs differently if using Python 3 type hints and the output doesn't look pretty. If it is selected to display the type hints at the beginning of the documentation for the object instead of in the declaration of the object it's just as ugly. However, if you turn off using the type hints all together and in the documentation you use the :type {param_name}: directive the output is much nicer. You will see this is what I have done.

I also set up a custom CSS file for the documentation. I increases the width and resets the margins to different size that scales to the width of the browser window. IDK why it is preset to be a fixed width but it is. parameters for functions and methods also appear on their own line and spaced properly to look like Python code. It makes it easier to read this way. My CSS programing ability is god enough to handle most things but for the life of me I was not able to determine if a series of parameters would surpass the width and break them up based on that. so even if the function/method has a single parameter and it would fit all on a single line it is still put onto it's own line. Small price to pay for readability.

There are a couple of options I set in the config file for the documentation and I fixed the error/warning for not locating "_static" as the repair of that was a side effect of adding the custom CSS file.

I made another commit and the new quantity system is now done. It is easily expanded/added onto by the user. I still have to key up documentation for it but if you read the rather long commit notes I go over how to use it when adding new quantities. Also check out the bottom of the example file to see some examples of how it works.

I found the bug that was causing inconsistent conversion results. I stumbled across the problem when I was modifying your implementation using __getattr__. The bug showed up when copying a unit. It was an indentation mistake LOL. Probably caused when moving code about using copy and paste, we both know how stupid PyCharm can be and I was probably foolish enough to expect it to do the right thing and I didn't check it over. I am the ID-10-T for not doing that.

@kdschlosser
Copy link
Contributor Author

kdschlosser commented Nov 29, 2021

Type hinting and autocomplete are now working for the Unit attributes in the units module.

@kdschlosser
Copy link
Contributor Author

I am looking at the scope of this unit converter and I am wondering if it should be it's own package/library. It's rather large in size and might be better if it was packaged by it's self. Either you or I can create an organization on GitHub for it and add the the person as an owner. Since this is a project both of us have time invested in and it contains ideas from both of us as well.

Just an idea/thought. lemme know what you think.

I wrote the operator methods over and expanded them to include true division, floor division, subtraction and addition. This allows for the use of all of the right hand and left hand math operators as well. I still have to add the bit shifting. I do not know if there is a way to dink about with the exponent operator I have never checked. I would assume there is.

I also did some work on the "units" module mechanics. I did not want users altering the actual stored units in any way. The way it is coded now it eliminates IDE errors for the attributes not being found for units with prefixes. The type hinting and autocomplete doesn't work for the units with prefixes and I do not believe there is a way to get them to work either. The new system also creates a new unit instance instead of letting the use alter the ones that are already stored.
@kdschlosser
Copy link
Contributor Author

@wolph

I have pushed some commits. Have a look I have made a bunch of changes. I think you will like the behavior.

This is kinda cool.

```python
mph = units.mph
kph = units.kph
mph /= kph

mph(20)
print(kph.value)

kph(150)
print(mph.value)
```

so when setting the value of one unit the cnoverted value shows up on the other.  Neat thing about that isit allows 2 remote sections of code in a program to work without actually passing anything between them. An example is say you have one block of code that hands the input and output of data. and that data is in kph, then you have another bunch of code that handles say a GUI interface. when the program initially starts a module level attribute can be created for the conversion using this technique. The data in and out uses the "kph" attribute and the gui uses the "mph" attribute. so the gui would do it's thing   and when updating the information it would  collect the value of "mph" the data end would set the data into "kph" which does the conversion and spits it out on the other end in the "mph" attribute.

It's kinda cool actually.

I can even write it so it supports more then a single conection. so if you wanted to say convert from km/h to mi/h, ft/s and m/s that could be done pretty easily. as it stands right now this can be done.

```python
kph = units.kph
kph.value = 60

kph.mph
kph.fps
kph.mps
```
and the returned value would be the converted value.
@wolph
Copy link
Owner

wolph commented Dec 6, 2021

Happy reading!!

And add the units to the units object so you can easily autocomplete and jump to the definition.

You don't actually have to add them to the dynamic module class. Because of the smoke and mirrors of how that whole thing works an IDE is not going to be able to trace that way through the program so what it sees is whatever is specified at the module level.

Perhaps not fully, but I think we might be able to get some support. Particularly, I think something like this might work, but I'm not entirely sure and I could be missing something:

units.atm = Unit('atm', '101325')

Can I give it a try? Or are you planning more updates?
If I succeed it will be a large update which is definitely going to give merge conflicts.

I have not yet created a bug report with JetBrains yet. Not that I would actually think they would fix it any time soon anyhow.

Yeah... I've tried several times. Don't bother.

If you're really lucky they might look at it in 5 years or so.

The really shitty thing is Sphinx outputs differently if using Python 3 type hints and the output doesn't look pretty. If it is selected to display the type hints at the beginning of the documentation for the object instead of in the declaration of the object it's just as ugly. However, if you turn off using the type hints all together and in the documentation you use the :type {param_name}: directive the output is much nicer. You will see this is what I have done.

I've seen similar issues but in my case it was largely caused by Sphinx not being able to find the correct import. But you're right, that's an issue.

Even though I'm not exactly a fan of generated code, using generated code in this case could be beneficial. In that case we could easily support both (and future variants) without any issues. For example, the type hinting system has switched back and forth from requiring an actual import to allowing strings over the years.

It would be possible to generate specific versions for newer/older Python versions.

I also set up a custom CSS file for the documentation. I increases the width and resets the margins to different size that scales to the width of the browser window. IDK why it is preset to be a fixed width but it is. parameters for functions and methods also appear on their own line and spaced properly to look like Python code. It makes it easier to read this way. My CSS programing ability is god enough to handle most things but for the life of me I was not able to determine if a series of parameters would surpass the width and break them up based on that. so even if the function/method has a single parameter and it would fit all on a single line it is still put onto it's own line. Small price to pay for readability.

My guess is that it's due to becoming difficult to read on large displays if the lines are too long:
image

As opposed to the old version:

image

I personally like the new version better, but it's getting a bit too wide perhaps. That can easily be solved with a max-width though. The old version is definitely too narrow but the new version might be a tad too wide for large displays.

Setting the max-width to 75% means that it will be very narrow on a small display so that's not ideal either. To illustrate, here's how the old version would look on an ipad:

image

And here's the new version:

image

But we can set it to a larger amount and be done with it :)
Or we can make these css rules media-dependent. On smaller screens they would be useless anyhow, so perhaps this should start to matter above 1200px width?

I made another commit and the new quantity system is now done. It is easily expanded/added onto by the user. I still have to key up documentation for it but if you read the rather long commit notes I go over how to use it when adding new quantities. Also check out the bottom of the example file to see some examples of how it works.

I find it weird how they call it quantities, that seems the wrong term for this... but a weird standard is better than no standard :)

I found the bug that was causing inconsistent conversion results. I stumbled across the problem when I was modifying your implementation using getattr. The bug showed up when copying a unit. It was an indentation mistake LOL. Probably caused when moving code about using copy and paste, we both know how stupid PyCharm can be and I was probably foolish enough to expect it to do the right thing and I didn't check it over. I am the ID-10-T for not doing that.

That's why I create a bunch of tests before changing stuff. If I break something while changing, I'll notice ;)
It's the safest way to refactor and/or restructure code from my experience

@kdschlosser
Copy link
Contributor Author

On large displays a person can simply window the browser and adjust the size to make the text fit how they want. That is why I changed both the margins and the widths to percentages instead of hard coded widths that do not adjust when the browser is resized. 😄

As far as the issue with the import and sphinx is concerned... I changed that whole thing up and I am no longer using .. :py:data:: anymore and instead I am using :cvar: as I have moved the units variables into the module class which is no longer the module class so the voodoo magic code has been removed in favor of using the import system to do the same thing but an IDE is able to trace a path through and the autocomplete and type hinting works correctly with. So there is no need for you to go through the pain staking process of declaring the units like you described.

IDK if you have tested or even looked at the code since The last bunch of commits. When you do you will see that the auto complete now works for all of the declared units. Once that are dynamically created there is no way to get the type hinting and auto complete to work, well in pycharm there isn't because pycharm doesn't pay attention to the return type set to __getattribute__ which is pretty dumb if you ask me. Tho I have not applied python 2 type hinting to it or sphinx type hinting either of those may do the trick with pycharm, I will have to test it out.

@kdschlosser
Copy link
Contributor Author

Oh also. the CSS code I added to put method and function parameters onto separate lines can be removed. I did that only because when sphinx add the type hinting to he parameters it makes a complete mess that is hard to read/understand. separating each parameter onto it's own line also takes the type with it making it much easier to read. I personally didn't care for how it looked with the type hinting and it also added confusion because this is how it would display it

__call__(
    value: Optional[Union[float, int, decimal.Decimal]] = None,
    factor: Union[float, int, decimal.Decimal] = 1.0,
    exponent: Union[int, decimal.Decimal] = 1
):
    blah blah blah blah

and I find this much easier on the eyes.

__call__(value=None, factor=1.0, exponent=1):

    value: value description
    type: Optional, float, int or decimal.Decimal

    factor: factor description
    type: Optional, float, int or decimal.Decimal
 
    exponent: exponent description
    type: Optional, int or decimal.Decimal

@kdschlosser
Copy link
Contributor Author

That's why I create a bunch of tests before changing stuff. If I break something while changing, I'll notice ;)
It's the safest way to refactor and/or restructure code from my experience

This is still in development and the API for it is not nailed down 100% yet and that is the reason why I hold off for creating unit tests. Once I get the API to a point where I know it is not going to change then I will make units tests up. making them up on an unstable and almost guaranteed to change API just makes more work because the units tests then have to be updated if the API changes and that just creates more work. If there is an error in the code the error will show up when the unit tests are created. for new development bug testing is the last thing I do. I do the bug testing and optimization at the same time. The reason why is because when I am looking over the code to find a bug I will almost always look at the code and find improvements that can be made as well. More times then not when I improve the code the bug gets sorted out as well. Point in fact, this bug that existed. I would have been tearing my hear out if I was trying to locate that bug. it would have been a needle in a haystack to locate, I happen to come across it during one of my code optimizing adventures and even when I first spotted it It didn't register as the cause of the problem you had. It didn't look right to me either but I couldn't quite put my finger on it. I had seen the problem you were having but I didn't change the code with the intention of fixing the bug. I changed the code to optimize it and the result was that bug went away. That is when I put 2 and 2 together.

@wolph
Copy link
Owner

wolph commented Dec 12, 2021

That's why I create a bunch of tests before changing stuff. If I break something while changing, I'll notice ;)
It's the safest way to refactor and/or restructure code from my experience

This is still in development and the API for it is not nailed down 100% yet and that is the reason why I hold off for creating unit tests. Once I get the API to a point where I know it is not going to change then I will make units tests up. making them up on an unstable and almost guaranteed to change API just makes more work because the units tests then have to be updated if the API changes and that just creates more work.

It depends on the type of test. I agree that many unit tests are not useful until you're done with all of the changes, but an integration test should return the same results.

Simply put, I would expect the results of a call like this to remain constant:

convert(52.0, '°F', '°C')

Or perhaps to make it even more stable:

round(convert(52.0, '°F', '°C'), 2)

A test like that will easily let you know if something broke. That's also how I noticed that the 2 different API's were unexpectedly giving different results.

As far as the issue with the import and sphinx is concerned... I changed that whole thing up and I am no longer using .. :py:data:: anymore and instead I am using :cvar: as I have moved the units variables into the module class which is no longer the module class so the voodoo magic code has been removed in favor of using the import system to do the same thing but an IDE is able to trace a path through and the autocomplete and type hinting works correctly with. So there is no need for you to go through the pain staking process of declaring the units like you described.

Ah, that's great :)
I was hoping to do it with a few regular expressions though, it shouldn't be pain staking... I hope

IDK if you have tested or even looked at the code since The last bunch of commits. When you do you will see that the auto complete now works for all of the declared units. Once that are dynamically created there is no way to get the type hinting and auto complete to work, well in pycharm there isn't because pycharm doesn't pay attention to the return type set to getattribute which is pretty dumb if you ask me. Tho I have not applied python 2 type hinting to it or sphinx type hinting either of those may do the trick with pycharm, I will have to test it out.

I looked at the commits but I haven't tested the code in an IDE (I mostly use neovim) so I didn't see any advanced autocompletion yet but I'll give it a try in pycharm.

I have an idea for the auto completion though, it should be easy enough with an auto-generated type stub (.pyi) file that's effectively the cartesian product of all unit combinations. I'll play around with it a bit :)

@kdschlosser
Copy link
Contributor Author

stub files aren't really needed. they are mainly for byte compiled ad extension modules. If your IDE is not able to autocomplete that is because the IDE you are using does not use Python 3 type hinting or sphinx type hinting to determine what the data type is. It only uses an express declaration in order to autocomplete properly.

What are you using for an autocomplete plugin ?? YouCompleteMe? You need to have a plugin for auto completes to work and to what extent I am not sure how much they will work.

@wolph
Copy link
Owner

wolph commented Dec 12, 2021

stub files aren't really needed. they are mainly for byte compiled ad extension modules. If your IDE is not able to autocomplete that is because the IDE you are using does not use Python 3 type hinting or sphinx type hinting to determine what the data type is. It only uses an express declaration in order to autocomplete properly.

As far as I know, it used to work around the __getattribute__ issue with pycharm. Not sure if it's still relevant.

What are you using for an autocomplete plugin ?? YouCompleteMe? You need to have a plugin for auto completes to work and to what extent I am not sure how much they will work.

I'm mostly using deoplete these days: https://github.com/WoLpH/dotfiles/blob/66642e18d4a2255773f8f37a13600ce61d1b1563/_vimrc#L381-L421

YouCompleteMe was far too slow for me in the past. Perhaps it's improved though, I should try again. Libraries do improve over time.

@kdschlosser
Copy link
Contributor Author

the __getattribute__ problem is only in detecting the type returned from the method. In PyCharm the hard coded class attributes autocomplete properly. It would be nice to have the dynamic ones work as well when use units.m all is good but units.cm autocomplete does not function because cm is dynamically created. To hard code the 500+ units each having 10 prefixes would make 5,000 hard coded class attributes. Little expensive to do that at runtime.

What is nice about using the __getattribute__ method is I am able to make a copy of the hard coded class attribute units this way a user is not able to modify the instances that have been set into those class attributes.

I have tried using Python 2 type hinting and also type hinting using sphinx and the auto complete does not work.

I did however change it up so the units class attributes get set directly with the proper unit instead of None and having the unit builder set the Unit instance to the class attribute. This is how you had wanted to go about doing it and I did it because the PyCharm debugging wasn't playing nice with setting it to None and then setting the instance later.

I still have to check to see if that fixed the debugging issue. I am going to open a bug report for PyCharm because of the type hinting not working properly with __getattr__ and __getattribute__. ya never know maybe they will fix it sooner then later.. LOL

@kdschlosser
Copy link
Contributor Author

I just confirmed making a stub file does not fix auto complete for units returned from __getattribute__

Repository owner deleted a comment from kloczek Jan 5, 2022
# Conflicts:
#	docs/conf.py
#	python_utils/time.py
#	setup.py
@wolph
Copy link
Owner

wolph commented May 29, 2022

I've just merged back the changes from develop and I've walked through the code quite a bit.

I think the code looks pretty good as it is. There's always room for improvement but I think it's time to just go for it and merge it :)
The only thing that still needs a little bit of work is the tests, but I'm looking at those as well.

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.

None yet

3 participants