Make `ProcessFlow::commodity_id` into `Rc<Commodity>`

[alexdewar]

This makes sense both to validate the input and also to provide better typing.

In order to implement this, we will probably have to rename the existing ProcessFlow struct into ProcessFlowRaw and provide a new ProcessRaw struct with the appropriate field, then write a function to convert between the two. See process.rs for similar examples.

[tsmbland]

@alexdewar I'm not sure I understand the point of this issue.

I think there's a bigger discussion to be had here about how we're dealing with references between tables (e.g. referencing the region in the ProcessRegion table etc.).

It looks like most of these are just using the object id as a String (although sometimes it's Rc<str>, e.g. region_id in the Asset struct)

I'm not sure I understand the difference and pros/cons of different methods, but I think we should be consistent

[alexdewar]

There are basically two reasons for this issue:

1. We have to check that the commodity IDs for each process flow are valid
2. Once we've done that, we'll know what the corresponding Commodity object is, so we might as well represent the field with this instead

Essentially, process flows will need a commodity field, which will have to point to something (even a String is pointing to some memory), so we might as well have it point to the actual commodity. There aren't really any advantages to having it just be a String or an Rc<str>; that just means if you want to know anything about the commodity you have to look it up somewhere else first.

The String vs Rc<str> is kind of a separate thing. Using Rc<str>s for IDs makes it cheap to copy them around (it also reduces memory usage, but this will be pretty negligible in our case) because you only have to increase the ref count rather than duplicating memory. We end up copying IDs around a fair bit in the input layer because most (all?) of the input files end up represented as a HashMap with an ID as the key. When you check whether an ID is valid or not you end up getting a reference to the Rc<str> so you might as well just use this and throw away the original String, assuming you are converting from a "raw" struct into something else anyway. (If you deserialise into an Rc<str> though, it will always allocate memory -- there's no global pool of Rc<str>s that's checked for an existing string -- so there's no advantage to using a String, which is why we're not just using Rc<str>s everywhere.)

At some point, if we want to optimise comparisons between either IDs (Rc<str>) or anything else ref counted (e.g. Rc<Commodity>), we could do this by just comparing the underlying pointer, which will be really fast (the Python equivalent is doing object1 is object2). The main reason for using Rcs for now though is just to avoid copies and complicated lifetimes, which is the alternative.

I haven't bothered using Rc<Region>s anywhere though because Regions only have an ID and a human-readable description, which we're presumably not going to need during the simulation stage. I'm not against doing it at some point though, if it is actually useful or just aligns with what we've done for other data structures.

[tsmbland]

Thanks @alexdewar that's really helpful!

Presumably though, for the same reason we want to make sure the commodity IDs in this case are valid, we'll want to do this for every ID reference. At which point, like you say, we might as well represent these fields with the actual object.

Maybe for most cases there would be no benefit of doing this at an individual level, but whatever we decide I'd still tend to advocate for consistency as it can make the code cleaner and easier to work with.

Down the line, I imagine we'll often want to select data according to the ID of certain fields (e.g. select all the processes for a specific region). Which way of representing the ID would make that kind of thing most efficient?

[alexdewar]

Presumably though, for the same reason we want to make sure the commodity IDs in this case are valid, we'll want to do this for every ID reference. At which point, like you say, we might as well represent these fields with the actual object.

Yep, probably. Sorry, I think I missed that point... One caveat is that we have to be careful not to make reference cycles (these aren't a problem in Python because of its clever garbage collector but can happen in Rust). So if you have an Rc<Process> with an Rc<Commodity> which has the same Rc<Process> as a member, then that Rc<Process> will never be dropped. Not sure how much of a risk this is, but it's something to keep in mind.

Maybe for most cases there would be no benefit of doing this at an individual level, but whatever we decide I'd still tend to advocate for consistency as it can make the code cleaner and easier to work with.

Agreed!

Down the line, I imagine we'll often want to select data according to the ID of certain fields (e.g. select all the processes for a specific region). Which way of representing the ID would make that kind of thing most efficient?

I don't think it makes much odds, but we'll want to compare the IDs for now. Either you'll have the commodity ID as a field (my_object.commodity_id) or you'll do my_object.commodity.id, which is just one extra pointer dereference, so a very minimal overhead. If you're doing one particular kind of lookup a lot though, you'll be better off caching the results (e.g. having a processes_by_region map).

The thing is though, there won't actually be that many processes, commodities, etc. in the grand scheme of things. If you needed to find processes for a particular region and you had a million of them, then you'd be silly not to cache the result, but I suspect there won't be a big performance difference for us. It's probably more important to choose APIs that are easy to work with for now. If we find performance bottlenecks in future, we can rework things a bit if needed.

[tsmbland]

Presumably though, for the same reason we want to make sure the commodity IDs in this case are valid, we'll want to do this for every ID reference. At which point, like you say, we might as well represent these fields with the actual object.

Yep, probably. Sorry, I think I missed that point... One caveat is that we have to be careful not to make reference cycles (these aren't a problem in Python because of its clever garbage collector but can happen in Rust). So if you have an Rc<Process> with an Rc<Commodity> which has the same Rc<Process> as a member, then that Rc<Process> will never be dropped. Not sure how much of a risk this is, but it's something to keep in mind.

I don't think there are any cycles in the current schema. If we're creating these references at the point of reading in each table, then I don't think this can happen anyway. E.g. Process references Commodity, so we'll have to read in the commodities first, then pass the commodities to the process reader to create the references. But a reference to Process couldn't be created in Commodity because the Process objects don't exist yet when Commodity is read in. Does that make sense?!

Maybe for most cases there would be no benefit of doing this at an individual level, but whatever we decide I'd still tend to advocate for consistency as it can make the code cleaner and easier to work with.

Agreed!

Down the line, I imagine we'll often want to select data according to the ID of certain fields (e.g. select all the processes for a specific region). Which way of representing the ID would make that kind of thing most efficient?

I don't think it makes much odds, but we'll want to compare the IDs for now. Either you'll have the commodity ID as a field (my_object.commodity_id) or you'll do my_object.commodity.id, which is just one extra pointer dereference, so a very minimal overhead. If you're doing one particular kind of lookup a lot though, you'll be better off caching the results (e.g. having a processes_by_region map).

The thing is though, there won't actually be that many processes, commodities, etc. in the grand scheme of things. If you needed to find processes for a particular region and you had a million of them, then you'd be silly not to cache the result, but I suspect there won't be a big performance difference for us. It's probably more important to choose APIs that are easy to work with for now. If we find performance bottlenecks in future, we can rework things a bit if needed.

Makes sense! I agree that choosing something easy to work with is the priority. Any opinions on this? Might be better to revisit this down the line when we have a better idea and then we can make things more consistent then.

[alexdewar]

I don't think there are any cycles in the current schema. If we're creating these references at the point of reading in each table, then I don't think this can happen anyway. E.g. Process references Commodity, so we'll have to read in the commodities first, then pass the commodities to the process reader to create the references. But a reference to Process couldn't be created in Commodity because the Process objects don't exist yet when Commodity is read in. Does that make sense?!

Yep. Sorry, bad example! I should have picked something more plausible... Anyways it's good that it shouldn't happen with the current schema.

Makes sense! I agree that choosing something easy to work with is the priority. Any opinions on this? Might be better to revisit this down the line when we have a better idea and then we can make things more consistent then.

Not really 😛. I think a "wait and see" approach is probably the best one. And maybe in the meantime we just try to replace instances of thing_id with Rc<Thing>s instead, because that seems like it'll be useful whatever path we go down.

In really general terms though, I think we could get a lot of mileage out of using iterators where we can, which can be quite expressive in Rust. To pick a nonsensical example, if you wanted to get the first process flow for every process with more than one PAC you can just do:

    for flow in model
        .processes
        .values()
        .filter(|process| process.pacs.len() > 1)
        .map(|process| process.flow.first())
    {
        // Do something
    }

In theory it should all compile down to something roughly the same as you'd get if you wrote a complicated loop in C instead, so performance will be good too.