santoshphilip / eppy

scripting language for E+, Energyplus
MIT License
151 stars 66 forks source link

Change design pattern of IDD #49

Open daren-thomas opened 9 years ago

daren-thomas commented 9 years ago

Currently, the IDD can only be set once - otherwise an IDDAlreadySetError shows up.

I think it would be more practical to have the IDD as a separate (possibly optional, see below) parameter to the IDF constructor. That way, you could have two IDF instances floating around that are bound to different IDD files - cache them by path name if that needs to be done for performance reasons.

This kind of functionality could be used, say, to create a pure python implementation of the version updater tool. Just one example.

If no IDD file is given, a simple function like this one could find the "default" IDD file in the system, given that EnergyPlus is installed and accessible via the systems PATH:

def find_idd():
    import os
    import distutils.spawn
    try:
        energyplus = distutils.spawn.find_executable('EnergyPlus')
        if not energyplus:
            raise DefaultIDDNotFoundError()
        energyplus = os.path.realpath(energyplus) # follow links in /usr/bin
        folder = os.path.dirname(energyplus)
        idd = os.path.join(folder, 'Energy+.idd')
        if not os.path.isfile(idd):
            raise DefaultIDDNotFoundError()
        return idd
    except:
        raise DefaultIDDNotFoundError()
santoshphilip commented 9 years ago

finding the default IDD is a good idea.

My first inclination was to ask you to insert that code into eppy. But I think I should do it, since that code will go into a place that is deep within eppy. It would be easier for me to do it.

Some questions about it:

Of course we could code it incrementally and add the functionality in steps

There are a couple of tricky issues with multiple IDDs, but is definitely possible. I'll write about these issues soon

santoshphilip commented 9 years ago

Multiple IDDs in eppy

Right now eppy works like this

It was written this way because the IDD code in eppy was developed in 2004. Computers were slower and had less memory in those days. The IDD file is large and held in memory. A design decision was made to use only one IDD in an instance of eppy.

Moving to multiple IDDs may be difficult since the expectation of having only one IDD is hard-coded into eppy. I have to revisit the code to see what is going in there. There is a slim chance that it may be trivially easy.

With multiple IDDs, the data structure would look like:

Some issues may arise when copying an object form idfv8_1 to idfv7_1, if the IDD definition of the objects is incompatible.

On the whole it would be worth moving in this direction and I'll start to put some thought into this

santoshphilip commented 9 years ago

API design for multiple IDDs

The API design may be as important as the coding when moving to multiple IDDs. I am almost inclined to something like a PEP, so I can get more input from others.

PEP stands for Python Enhancement Proposal that the python development team uses. You can read about PEP here:

Maybe we need an EEP (eppy enhancement proposal :-) Just a thought here

daren-thomas commented 9 years ago

As far as I can tell, the find_idd function should be portable. But I really haven't tried. The way it works (distutils.spawn.find_executable) is by going through the $PATH environment variable looking for the first EnergyPlus executable. On Windows, this will pick up EnergyPlus.exe. The beauty of doing it this way is that it picks up the version of EnergyPlus that the user has configured to be the "default" (given that the user knows how to do this - but also, when installing EnergyPlus, this tends to be done automatically, at least on windows). On my system it picks up my fork in my projects directory.

I really like your other ideas and I think you are right: We should spend some time to get the spec right before making any fixes.

I would have supplied code for the change for multiple IDDs if I had figured it out - but as you say, it is deep in the guts. This is often a problem with the singleton design pattern (of which this is an instance) but can maybe be fixed by finding all (global) references to the IDD in the IDF code and changing those to an instance attribute in the IDF object.

I hadn't even thought about problems created when copying one object from one idf to another. Wow. Yes, that would have to be checked / guarded...

If you need help designing / writing this, just let me know. I'm on a tight schedule through September, but will try to help as much as possible.

daren-thomas commented 9 years ago

I have updated the find_idd function above to follow symlinks as produced by the darwin installer in /usr/bin - that way, the current /usr/bin/EnergyPlus is resolved to /Applications/EnergyPlus-8-3-0/energyplus-8.3.0 and the parent folder contains the Energy+.idd file. I think this should work for Linux too, but have not had a chance to test yet.

Also, I think while this is nice to have when it works, if it does break down, the user is still not worse off than when he started...

santoshphilip commented 9 years ago

This issue is not mission critical. But definitely nice to have. The September time line is fine. We could work back and forth on it. I'll take a quick stab at putting find_idd and test it a bit.

I have started the beginnings of a developer documentation: http://pythonhosted.org/eppy/dev_docs/index.html Not as good as the Tutorial, since I did not have Leora's help in doing it. (Leora being the co-author of the tutorial)

I could use this issue as an opportunity to articulate the dev-docs better, so it would be easier for you to work on the code, and hence easier for other developers to work on it too. In the long run that may be a good use of my time, rather than doing all the coding, simply because it is quicker if I do it at present.

Also take a look at issue #50 Jaimie has started the first EEP! (Eppy Enhancement Proposal)

santoshphilip commented 8 years ago

Darren,

I have implemented your function _findidd() in branch i41_idfnew. follow conversation in #41 if you need to as I am making other changes connected to this.

I have used you code exactly as is. What is your full name, so I can add it in the copyright in that file ?

Santosh

daren-thomas commented 8 years ago

Santosh,

Yay! You just made my monday :)

If you want to add my full name to the copyright file, use this:

Daren Thomas

  • just one "r" in Daren, not two...
santoshphilip commented 8 years ago

Can you put some thought into how to unit test it. (eppy uses pytest) I don't yet have a sense of how to do it.

daren-thomas commented 8 years ago

Santosh, do you still need "some thought [put] into how to unit test it"? Without thinking too much, here are some thoughts:

The find_idd function interfaces with the os. That always makes unit testing difficult! One way to test it, if you really need to would be to mock the distutils.spawn.find_executable('EnergyPlus') call. Pass in the function to find EnergyPlus as a parameter, with distutils.spawn.find_executable as the default. Then, in the unit test, you can provide a known location (to a dummy file) with test data.

This does not really solve testing symlinks etc. Personally, I wouldn't write a test for this.

santoshphilip commented 8 years ago

Darren, I am just running a little behind on this, but I think I have a good handle on it.

I went for a python meet up and got some good mentoring/guidance on this. My thinking on this is the following:

When I get a moment of time, I'll implement this :-)