search

NTNU-IndEcol / BuildME

6 stars 1 forks source link

Pre-compute MMV archetypes #30

Closed nheeren closed 2 years ago

nheeren commented 2 years ago

@kamilitsa Again thank you for contributing the MMV functionality!

While I am still trying to understand all of it, I noticed that the computation of the MMV variant is computationally quite expensive. So I was wondering if this step could not happen beforehand. I created a new file pre.py for doing these types of pre-computations.

However, I am not sure if this will work on the template idf files in the archetype folder, i.e. before the replace-me strings have been replaced. Do you think this would be possible?

Would you be motivated to flesh out the function pre.create_mmv_variants()? https://github.com/nheeren/BuildME/blob/079d1fed860df5e74727c635d46aa918504ac935/BuildME/pre.py#L47-L49

kamilakrych commented 2 years ago

@nheeren If I understand correctly, then the thing you're asking me to do is already implemented 😉

The highlighted line is the one where the archetype is changed to the MMV type. The line below is where replace-me strings are handled. The E+ simulation is indeed quite computationally intensive, but it's done as usual, independently of the MMV variant.

image

Maybe I misunderstood, if yes, then please clarify.

nheeren commented 2 years ago

Sorry, I was not being 100% clear. But you answered the important question, i.e. if the conversion can happen with the unspecified file (Yes).

I was thinking that the BuildME script should do the following:

  1. Check what MMV archetypes are requested in settings.combinations
  2. Check if these MMV archetypes are already in ./data/archetypes, e.g. ./data/archetype/USA/MFH_*MMV*.idf.
  3. If not, run mmv.change_archetype_to_MMV() on each and place them into ./data/archetypes, i.e. not ./tmp
  4. Now start the real work and do what BuildME does...

So by pre-computing the MMV archetypes only once, we could save a lot of computation time, when somebody needs to run it more than once.

Does that make more sense?

kamilakrych commented 2 years ago

Oh okay, thanks for clarification 😃

Yes, that would be a nice improvement indeed! I will try to implement it within the next days.

nheeren commented 2 years ago

Thanks! Let me know if you don't find the time. I am working on the code a bit anyway. Make sure to pull for the new pre.py 😉

nheeren commented 2 years ago

Maybe we can also add some kind of note to the idf file to make it clear that it has been auto-generated. For instance a string at the beginning of the file like:

!- Auto-generated from ./data/archetype/USA/MFH.idf by BuildME

or append ./data/archetype/USA/MFH_*MMV-auto*.idf.

kamilakrych commented 2 years ago

Done! See this commit: 3d4295b4fcac4fd7269b88dfb013ccb67e378513

PS I have noticed only now that you have created "Babysteps towards pre-computation of MMC", so I sadly didn't use it at all...

nheeren commented 2 years ago

Great! Looking forward to check it out soon. I wonder if we should add *_mmv.idf to the .gitignore file, because they are a function of the original .idf. Any thoughts?

kamilakrych commented 2 years ago

I didn't know about the gitignore option, but it would make sense to use it in this case, yes :)

nheeren commented 2 years ago

You want to add it? Will you make a pull request?

kamilakrych commented 2 years ago

I just did! Sorry for the small mess there, I'm still learning how to use git. I made the MMV changes on top of Brazilian office building archetypes, so I reverted some changes not to mix up the pull requests.