Planet-Factory / legacy-claude

The CLAuDE model
https://planet-factory.github.io/claude-docs/
MIT License
167 stars 28 forks source link

Separate Config File from Code #25

Closed jamstruth closed 2 years ago

jamstruth commented 3 years ago

This request separates the constants out into YAML config files in order to allow for adjusting how the model works without requiring edits directly to Python code. This has been done in an Object-Oriented manner using PyYAML's available mapping facilities. Tests have been added to ensure that loading works as expected.

Toy_Model has then been updated to load the "Default" config file, convert into the Config objects then all references to the previous constants have been updated to refer to the Config Objects. I have tested that the model still launches successfully after these changes.

WardPearce commented 3 years ago

Just some changes I'd personally make.

https://github.com/jamstruth/claude/blob/feature/object_oriented_claude/model/claude_config.py#L8 wildcard imports aren't the best idea.

https://github.com/jamstruth/claude/blob/feature/object_oriented_claude/model/claude_config.py#L29

def __eq__(self, other):
    return isinstance(other, PlanetConfig) and (self.day == other.day and self.year == other.year
                                                and self.resolution == other.resolution
                                                and self.planet_radius == other.planet_radius
                                                and self.insolation == other.insolation
                                                and self.gravity == other.gravity
                                                and self.axial_tilt == other.axial_tilt
                                                and np.array_equal(self.pressure_levels, other.pressure_levels)
                                                and self.nlevels == other.nlevels)

I'd do the same with https://github.com/jamstruth/claude/blob/feature/object_oriented_claude/model/claude_config.py#L184

Not 100% sure why you've used bitwise operators in places, e.g. & (Sets each bit to 1 if both bits are 1)

jamstruth commented 3 years ago

I'll clean up those wildcard imports and the & operators. The & was habit to be honest, I'm used to Java which uses && as it's logical and operator.

As for the equals I had written it for readability even if its slightly less efficient as a result. I don't expect checking parity between config objects to come up outside of tests so speed isn't a big factor there.

WardPearce commented 3 years ago

I'll clean up those wildcard imports and the & operators. The & was habit to be honest, I'm used to Java which uses && as it's logical and operator.

As for the equals I had written it for readability even if its slightly less efficient as a result. I don't expect checking parity between config objects to come up outside of tests so speed isn't a big factor there.

yeah I assumed as much, python can be annoying to write after coming back from writing pretty much any language lol.

Personally the changes I suggested are more readable to me, but that's just a preference.

jamstruth commented 3 years ago

Updated code for small cleanup suggestions

ATGSilva commented 2 years ago

Stale