Download stops after first blob, payment sent, restarts in stopped status

The Issue
This has happened to me twice (update, now 3 times!) now, not sure how to reproduce it consistently.
2018-04-27 15:08:20,618 INFO     lbrynet.daemon.Downloader:93: Download lbry://two status changed to initializing
2018-04-27 15:08:20,624 INFO     lbrynet.daemon.ExchangeRateManager:221: Converting 50.000000 LBC to LBC, rates: [Currency pair:('USD', 'BTC'), spot:0.000108066589509, ts:1524856044, Currency pair:('BTC', 'LBC'), spot:29350.7670639, ts:1524856044, Currency pair:('BTC', 'LBC'), spot:29351.9505936, ts:1524856044, Currency pair:('USD', 'BTC'), spot:0.00010799, ts:1524855145, Currency pair:('BTC', 'LBC'), spot:29239.7660819, ts:1524856045]
2018-04-27 15:08:20,624 INFO     lbrynet.daemon.ExchangeRateManager:221: Converting 10.000000 LBC to LBC, rates: [Currency pair:('USD', 'BTC'), spot:0.000108066589509, ts:1524856044, Currency pair:('BTC', 'LBC'), spot:29350.7670639, ts:1524856044, Currency pair:('BTC', 'LBC'), spot:29351.9505936, ts:1524856044, Currency pair:('USD', 'BTC'), spot:0.00010799, ts:1524855145, Currency pair:('BTC', 'LBC'), spot:29239.7660819, ts:1524856045]
2018-04-27 15:08:20,625 INFO     lbrynet.daemon.Downloader:93: Download lbry://two status changed to downloading_metadata
2018-04-27 15:08:20,973 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-04-27 15:08:20,973 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob f204bb18354b803c17ecbfd86e796a3ec4fd0097506812a97189a6ffe6f2a9f41dcfd02463f33a587015ffef870bf0fe from <IP_ADDRESS>:3333
2018-04-27 15:08:21,086 INFO     lbrynet.core.client.BlobRequester:563: Blob f204bb18354b803c has been successfully downloaded from <IP_ADDRESS>:3333
2018-04-27 15:08:21,638 INFO     lbrynet.daemon.Downloader:132: Pay key fee 10.000000 --> bVxoPVM3HmbiE2yhB8RsFcViSK6RSj6kX9
2018-04-27 15:08:21,654 INFO     lbrynet.daemon.Downloader:186: Downloading lbry://two (f204bb) --> C:\Users\SoloDolo\Downloads\lbry downloads\Mystery-Box-1.zip
2018-04-27 15:08:21,654 INFO     lbrynet.daemon.Downloader:93: Download lbry://two status changed to running
2018-04-27 15:08:21,934 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-04-27 15:08:21,934 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob 8341e24dc7b56493d0d941830466fa93b0ed5f449c4cf3a50e8c6db4885650de7e7d216fc11d87ec80cd23cd6f7ffc45 from <IP_ADDRESS>:3333
2018-04-27 15:09:00,469 INFO     lbrynet.core.Wallet:310: Skipping dust
2018-04-27 15:09:00,549 INFO     root:205: using buckets: [u'ff4ed37a622dfbd9eb873613d218b2d1a2c3a8053f0288bdd66e5c26f9ef19db:0', u'7c75918f31a2df058ae633b27c9625c31bb4dc106725d414d7c25dd1dcf2e075:1']

Nothing happens after the last blob is requested. When restarting daemon, the file is in stopped status so it doensn't  ever complete downloading. Running a file_set_status start on the sd_hash it finishes.
  "blobs_completed": null,
    "blobs_in_stream": null,
    "channel_claim_id": "deeb66c6f87e637747f88028ce77d16b9ba67d5e",
    "channel_name": "@mystery-box",
    "claim_id": "eee58ceec76059fd2d6b36dbddf0b5faca408130",
    "claim_name": "two",
    "completed": false,
    "download_directory": "C:\\Users\\xxx\\Downloads\\lbry downloads",
    "download_path": "C:\\Users\\xxx\\Downloads\\lbry downloads\\Mystery-Box-1.zip",
    "file_name": "Mystery-Box-1.zip",
    "key": "a075f4138365a717e3f924e07e428b25",
    "metadata": {
      "author": "",
      "description": "Secret Mystery Box - What goodies are inside? Find out.\n\n\nSometimes these boxes contain amazing gifts, sometimes normal gifts and sometimes nothing.",
      "fee": {
        "address": "bVxoPVM3HmbiE2yhB8RsFcViSK6RSj6kX9",
        "amount": 10.0,
        "currency": "LBC",
        "version": "_0_0_1"
      },
      "language": "en",
      "license": "Public Domain",
      "licenseUrl": "",
      "nsfw": false,
      "preview": "",
      "thumbnail": "https://spee.ch/6/mystery-box.jpeg",
      "title": "Secret Mystery Box",
      "version": "_0_1_0"
    },
    "mime_type": "application/x-zip-compressed",
    "nout": 0,
    "outpoint": "040e43e33d55bd7aba2ec0e1c355206de18ce031e2ca964a7c5068b3a1f56a9e:0",
    "points_paid": 0.0,
    "sd_hash": "f204bb18354b803c17ecbfd86e796a3ec4fd0097506812a97189a6ffe6f2a9f41dcfd02463f33a587015ffef870bf0fe",
    "status": null,
    "stopped": true,
    "stream_hash": "08657cbdfbd74a15f4ab68b915a42a808aa059a35116783d3e7a5088750e65f2c6ac3df1f8bda0725e2d876b2832746f",
    "stream_name": "Mystery-Box-1.zip",
    "suggested_file_name": "Mystery-Box-1.zip",
    "total_bytes": null,
    "txid": "040e43e33d55bd7aba2ec0e1c355206de18ce031e2ca964a7c5068b3a1f56a9e",
    "written_bytes": 0
  }

Steps to reproduce

Expected behaviour
Tell us what should happen
Actual behaviour
Tell us what happens instead
System Configuration

LBRY Daemon version: 0.19.1
LBRY App version:
LBRY Installation ID:
Operating system:

Anything Else

Screenshots

Happened to me again:
2018-05-31 17:30:13,914 INFO     lbrynet.daemon.Downloader:93: Download lbry://theperfecthouse status changed to downloading_metadata
2018-05-31 17:30:14,579 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-05-31 17:30:14,581 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob 409a214fbc412ccbe87c584340bbb150eb0cc9a8e81d090ff47ed0c469c4b8ba21be25b3593d35d5938a54a1d711c03a from <IP_ADDRESS>:3352
2018-05-31 17:30:14,588 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-05-31 17:30:14,588 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob 409a214fbc412ccbe87c584340bbb150eb0cc9a8e81d090ff47ed0c469c4b8ba21be25b3593d35d5938a54a1d711c03a from <IP_ADDRESS>:3333
2018-05-31 17:30:14,609 INFO     lbrynet.core.client.BlobRequester:563: Blob 409a214fbc412ccb has been successfully downloaded from <IP_ADDRESS>:3352
2018-05-31 17:30:14,634 INFO     lbrynet.core.client.ClientProtocol:185: Closing the connection to <IP_ADDRESS>:3333 because the download of blob 409a214fbc412ccb was canceled
2018-05-31 17:30:15,566 INFO     lbrynet.daemon.Daemon:1678: Already waiting on lbry://theperfecthouse to start downloading
2018-05-31 17:30:16,563 INFO     lbrynet.daemon.Downloader:132: Pay key fee 5.405147 --> bCvxVJX92FQ54UVN2BbbUeYUqVJusf5ynL
2018-05-31 17:30:16,573 INFO     lbrynet.daemon.Downloader:186: Downloading lbry://theperfecthouse (409a21) --> c:\lbry4\Downloads\The Perfect House 1080.mp4
2018-05-31 17:30:16,575 INFO     lbrynet.daemon.Downloader:93: Download lbry://theperfecthouse status changed to running
2018-05-31 17:30:16,875 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-05-31 17:30:16,881 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob 3db2f9b5626f705fb22b0034ea99bd548871a25086e69cd9ef5bfee6ca4d787422f53467cc70cecc4f10d668d653c8ef from <IP_ADDRESS>:3352
2018-05-31 17:31:06,674 INFO     lbrynet.core.Wallet:395: Skipping dust
2018-05-31 17:31:06,674 INFO     lbrynet.core.Wallet:395: Skipping dust

No timeout, sitting in stopped = true, completed =false
Upon shutting down the daemon, did get this:
Unhandled error in Deferred:
2018-05-31 18:03:25,729 CRITICAL twisted:154: Unhandled error in Deferred:

2018-05-31 18:03:25,747 CRITICAL twisted:154:
Traceback (most recent call last):
  File "site-packages\twisted\internet\defer.py", line 1299, in _inlineCallbacks
  File "site-packages\lbrynet\daemon\Daemon.py", line 671, in _download_failed
  File "site-packages\lbrynet\analytics.py", line 61, in send_download_errored
  File "site-packages\lbrynet\analytics.py", line 182, in _download_error_properties
AttributeError: Failure instance has no attribute 'message'

Happened again:
2018-06-17 13:29:16,984 INFO     lbrynet.daemon.Downloader:93: Download lbry://three status changed to initializing
2018-06-17 13:29:16,987 INFO     lbrynet.daemon.ExchangeRateManager:228: Converting 50.000000 LBC to LBC, rates: [Currency pair:('USD', 'BTC'), spot:0.000153817283525, ts:1529256465, Currency pair:('BTC', 'LBC'), spot:56575.2701717, ts:1529256465, Currency pair:('BTC', 'LBC'), spot:56582.3860262, ts:1529256466]
2018-06-17 13:29:16,987 INFO     lbrynet.daemon.ExchangeRateManager:228: Converting 10.000000 LBC to LBC, rates: [Currency pair:('USD', 'BTC'), spot:0.000153817283525, ts:1529256465, Currency pair:('BTC', 'LBC'), spot:56575.2701717, ts:1529256465, Currency pair:('BTC', 'LBC'), spot:56582.3860262, ts:1529256466]
2018-06-17 13:29:16,987 INFO     lbrynet.daemon.Downloader:93: Download lbry://three status changed to downloading_metadata
2018-06-17 13:30:50,392 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-06-17 13:30:50,392 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob 348d5fd8faa643bfd96e5eb69196c3e2969e48f12bbac1aa4f1f6536a6dea4f1ed943f51ae758f7ee54a7371e136c659 from <IP_ADDRESS>:3333
2018-06-17 13:30:50,430 INFO     lbrynet.core.client.BlobRequester:563: Blob 348d5fd8faa643bf has been successfully downloaded from <IP_ADDRESS>:3333
2018-06-17 13:30:51,081 INFO     lbrynet.daemon.Downloader:132: Pay key fee 10.000000 --> bZYUVLcKXhVYsFQwYqPEJQ9zan8KwZB8dG
2018-06-17 13:30:51,086 INFO     lbrynet.daemon.Downloader:186: Downloading lbry://three (348d5f) --> C:\Users\SoloDolo\Downloads\lbry downloadstest\Carvana Image Masking Challenge.pdf
2018-06-17 13:30:51,086 INFO     lbrynet.daemon.Downloader:93: Download lbry://three status changed to running
2018-06-17 13:30:57,387 INFO     lbrynet.core.client.BlobRequester:414: Offered rate 0.000000/mb accepted by <IP_ADDRESS>
2018-06-17 13:30:57,388 INFO     lbrynet.core.client.BlobRequester:487: Requesting blob 340d8e5fc8379b5f8953ee64c603583381523456296d2cd05949500678b222c43e50fa36e530eb2790c2bebccc097f79 from <IP_ADDRESS>:3333
2018-06-17 13:31:11,608 INFO     lbrynet.dht.hashannouncer:54: Announcing 1 blobs
2018-06-17 13:31:25,631 INFO     lbrynet.dht.hashannouncer:72: Took 14.0230000019 seconds to announce 1 of 1 attempted hashes (0.000000 hashes per second)
2018-06-17 13:31:45,319 INFO     lbrynet.core.Wallet:410: Skipping dust
2018-06-17 13:31:45,323 INFO     root:205: using buckets: [u'f7065aa34b48ccc81bbc52ee7b74fd4b10103280f2892ec894c945eca482c3c6:1']
2018-06-17 13:31:47,904 ERROR    lbryum:119: [lbryumx2.lbry.io] serving improperly sorted address histories

2018-06-17 13:34:41,608 INFO     lbrynet.dht.hashannouncer:54: Announcing 1 blobs
2018-06-17 13:34:47,081 INFO     lbrynet.dht.hashannouncer:72: Took 5.47199988365 seconds to announce 1 of 1 attempted hashes (0.000000 hashes per second)
2018-06-17 13:35:11,609 INFO     lbrynet.dht.hashannouncer:54: Announcing 1 blobs
2018-06-17 13:35:20,851 INFO     lbrynet.dht.hashannouncer:72: Took 9.24099993706 seconds to announce 1 of 1 attempted hashes (0.000000 hashes per second)

  {
    "blobs_completed": null,
    "blobs_in_stream": null,
    "channel_claim_id": null,
    "channel_name": null,
    "claim_id": "562d308abdc1f815dd2933bc681e74b410cb6c2c",
    "claim_name": "three",
    "completed": false,
    "download_directory": "C:\\Users\\SoloDolo\\Downloads\\lbry downloadstest\\",
    "download_path": "C:\\Users\\SoloDolo\\Downloads\\lbry downloadstest\\Carvana Image Masking Challenge.pdf",
    "file_name": "Carvana Image Masking Challenge.pdf",
    "key": "cf76009b20e1f12f26ef2aeffc2a54cf",
    "metadata": {
      "author": "",
      "description": "Carvana Image Masking Challenge",
      "fee": {
        "address": "bZYUVLcKXhVYsFQwYqPEJQ9zan8KwZB8dG",
        "amount": 10.0,
        "currency": "LBC",
        "version": "_0_0_1"
      },
      "language": "en",
      "license": "None",
      "licenseUrl": "",
      "nsfw": false,
      "preview": "",
      "thumbnail": "https://spee.ch/f/kaggle.jpeg",
      "title": "kaggle",
      "version": "_0_1_0"
    },
    "mime_type": "application/pdf",
    "nout": 0,
    "outpoint": "ec2265df795a7cc3c0a87180ce0e98a6a0cc5227e3f50d8cfd3940e4f6efa021:0",
    "points_paid": 0.0,
    "sd_hash": "348d5fd8faa643bfd96e5eb69196c3e2969e48f12bbac1aa4f1f6536a6dea4f1ed943f51ae758f7ee54a7371e136c659",
    "status": null,
    "stopped": true,
    "stream_hash": "bb5c6289112f039292bdb212a0b277494e29081fa4d0ff57cccf997a510d1bf5b1dc9b5ac0eeebd9930c2b0c7644f4c6",
    "stream_name": "Carvana Image Masking Challenge.pdf",
    "suggested_file_name": "Carvana Image Masking Challenge.pdf",
    "total_bytes": null,
    "txid": "ec2265df795a7cc3c0a87180ce0e98a6a0cc5227e3f50d8cfd3940e4f6efa021",
    "written_bytes": 0
  }
]

Reopening, ran into this after the above commit was merged. Will keep an eye out if it happens again. File ended up in stopped status.
C:\Users\SoloDolo\Desktop\latest daemon>lbrynet-cli file_list --claim_id=562d308abdc1f815dd2933bc681e74b410cb6c2c
[
  {
    "blobs_completed": null,
    "blobs_in_stream": null,
    "channel_claim_id": null,
    "channel_name": null,
    "claim_id": "562d308abdc1f815dd2933bc681e74b410cb6c2c",
    "claim_name": "three",
    "completed": false,
    "download_directory": "C:\\Users\\SoloDolo\\Downloads\\lbry downloadstest\\",
    "download_path": "C:\\Users\\SoloDolo\\Downloads\\lbry downloadstest\\Carvana Image Masking Challenge.pdf",
    "file_name": "Carvana Image Masking Challenge.pdf",
    "key": "cf76009b20e1f12f26ef2aeffc2a54cf",
    "metadata": {
      "author": "",
      "description": "Carvana Image Masking Challenge",
      "fee": {
        "address": "bZYUVLcKXhVYsFQwYqPEJQ9zan8KwZB8dG",
        "amount": 10.0,
        "currency": "LBC",
        "version": "_0_0_1"
      },
      "language": "en",
      "license": "None",
      "licenseUrl": "",
      "nsfw": false,
      "preview": "",
      "thumbnail": "https://spee.ch/f/kaggle.jpeg",
      "title": "kaggle",
      "version": "_0_1_0"
    },
    "mime_type": "application/pdf",
    "nout": 0,
    "outpoint": "ec2265df795a7cc3c0a87180ce0e98a6a0cc5227e3f50d8cfd3940e4f6efa021:0",
    "points_paid": 0.0,
    "sd_hash": "348d5fd8faa643bfd96e5eb69196c3e2969e48f12bbac1aa4f1f6536a6dea4f1ed943f51ae758f7ee54a7371e136c659",
    "status": null,
    "stopped": true,
    "stream_hash": "bb5c6289112f039292bdb212a0b277494e29081fa4d0ff57cccf997a510d1bf5b1dc9b5ac0eeebd9930c2b0c7644f4c6",
    "stream_name": "Carvana Image Masking Challenge.pdf",
    "suggested_file_name": "Carvana Image Masking Challenge.pdf",
    "total_bytes": null,
    "txid": "ec2265df795a7cc3c0a87180ce0e98a6a0cc5227e3f50d8cfd3940e4f6efa021",
    "written_bytes": 0
  }

Happened to me again...


made the executable into a module

I had to modify your version so I can use it from Haskell src, pasting it here, you can merge it with yours if you want.

This looks pretty good, but please re-sync with my latest patch which I just pushed.

Sorry for the flail. Here's the ref you'll probably want to work from a78a8cfd66603e2384a171fd974e25d842330c72

uhhhh read that after pushing the module-only version

Is there a proper namespace for stuff like this, ie: Network.Imgur?

@ Network.Imgur, I have no idea, merge it, possibly change it later


entrypoint fails with non-root user

Launching the last version of the containers is failing if another user than root is used.
The entrypoint specified is in /root/ and unreachable for normal users, and the command fails.
Using --entrypoint '' is a workaround, but then the environment is not setup correctly (maybe the issue of #14 ?).
The script to source should be placed somewhere reachable for all users (/tmp or with the other intel stuff in /opt, with correct permissions).
I personally preferred the previous version with ENV commands in the dockerfile, even if verbose, as they left the entrypoint free for other usages in containers using these as base.

Hello @adegomme ,
Please find the latest OneAPI images (no entrypoint used in any oneapi container image, switch to ENV to avoid the scenario you planted)
Kind regards

As @luisfponce wrote, newer images do not have entrypoints set and went back to ENV style setup.
Closing this issue.


Fix benchmark script

@min-xu-et

Oh, I checked git blame and figure it out. The bug was mine. Sorry about that. The reqs var was created for the warmup and the buggy code keeps on using it. Great catch!!


Add readme and doc stubs

@dirkschumacher not sure why this is failing. I did not make any code changes. Is develop currently broken?


Implementation done for #97

Implemented the add product page

@all-contributors please add @AdrishOfHogwarts for code


Make MCDA functions generic

Make MCDA functions generic to any number of criteria, and use auto-generated names as opposed to the current index-based approach which is prone to errors when making adjustments.

create_decision_matrix
create_seed_matrix

Could make changes to adopt JMcDM as suggested in #242 instead.

@ConnectedSystems
Thoughts on the best way to make the MCDA functions accept any number of criteria with auto-generated names. I'm imagining this as a series of changes:

Change the mcda_vars constructor to create a table of criteria of size (nsites *ncriteria) with column names generated from the names of the variables it was constructed from, and a dict of weights of size ncriteria.
Change the decision matrix construction to just use the constructed table and weights to form a matrix, with normalisation.
The seeding and shading matrix contructors would then just select from this matrix based on some info about which columns are seeding criteria and which are shading (and later fogging, when these are separated).
Criteria which require more complicated calculations, such as the space left over for coral growth at each site,  could be calculated in separate functions outside of the matrix construction and then fed into the mcda_vars constructor so that the decision matrix constructor could remain generic.

How did you imagine a restructure to allow more genericity? The MCDA techniques package could be cool to incoporate, allowing the use of more algorithms (although this would have to be restricted in sensitivity analyses).

Change the mcda_vars constructor to create a table of criteria of size (nsites *ncriteria)

I'm assuming here that "mcda_vars" refers to the DMCDA_vars struct.
It seems DMCDA_vars could be used more effectively. It's largely used as a data container where, in effect, variables are simply extracted and renamed.
For example, in guided_site_selection(), there's a series of lines similar to:
strong_pred = d_vars.strong_pred[site_ids, :]
in_conn = d_vars.in_conn[site_ids]
out_conn = d_vars.out_conn[site_ids]
...

I'm not 100% certain yet, but it also seems the variables included in DMCDA_vars are not completely related to each other.
If instead this becomes a table-like data structure, how would it deal with the fields that appear to be tables themselves (e.g., strong_pred)

... with column names generated from the names of the variables it was constructed from, and a dict of weights of size ncriteria.

I agree in principle with this, yes.
The pattern can be repeated elsewhere as well, for example:
create_decision_matrix(site_ids, in_conn, out_conn, sum_cover, max_cover, area, wave_stress, heat_stress, site_depth, predec, zones_criteria, risk_tol)

can simply be:
create_decision_matrix(site_ids; other_args...)

I'm sure there are other, more direct, approaches we could use here, but it's what comes to mind when I look at it.

The seeding and shading matrix contructors would then just select from this matrix based on some info about which columns are seeding criteria and which are shading (and later fogging, when these are separated).

Okay, so we should resolve #277 first?

Criteria which require more complicated calculations, such as the space left over for coral growth at each site, could be calculated in separate functions outside of the matrix construction and then fed into the mcda_vars constructor so that the decision matrix constructor could remain generic.

I agree that calculations ancillary to the purpose of constructing the decision matrix should be moved elsewhere, perhaps into new functions that accept DMCDA_vars as an input. Right now, responsibility/concerns are not well separated.

Yeah sorry "mcda_vars" refers to the DMCDA_vars struct.
I think the criteria which include 'table like' structures would have to be constructed/calculated elsewhere before being used in constructing the table. All the criteria are eventually processed into vectors of length nsites, its just some of them begin with more dimensions (such as strong_pred, which is used to construct the priority sites criterium).
We could resolve #277 first but will fogging be treated differently in terms of criteria and weightings? If not, it won't matter so much, but if it will be then we should probably resolve that first. I can imagine fogging and SRM having slightly different criteria, but I could ask Ken to see what he thinks?

will fogging be treated differently in terms of criteria and weightings?

Ah, maybe in this context "shading" will simply be renamed to "fogging"?
I'm not sure what "shading" will mean in the future (i.e., at larger spatial scales). If shading is active, does that mean we assume all of GBR is shaded, or do we target a specific set of reef clusters (e.g., we shade "the north" or "the south", whatever that may mean).

Sorry, where I've said SRM should say shading. But yes, maybe in this context shading can be renamed to fogging...

Did we decide we need to separate shading and fogging before approaching this issue or is it fine if I begin on this and then implement any necessary changes for adding fogging as separate later? I'm thinking the changes will not be too dramatic if I make the functions sufficiently generic.

For the MCDA the change for #277 may be (and I stress may be) simply renaming "shading" to "fogging" as appropriate but for #277 to we also need to make sure there are no other changes required for model runs where shading variables could be mixed in with those for fogging.
Additionally, has there been any discussion on how SRM will be represented at larger spatial scales?
As mentioned above, currently we apply benefits domain-wide but this may not be plausible if a larger spatial scale is represented.

Right, maybe something to discuss with Ken on the 16th. I remember having some very general conversations with Ken about SRM on larger scales but it would be good to revisit given how much ADRIA has changed since then.

Adding to this discussion here to flesh out planned changes to make MCDA more generic @ConnectedSystems

Plan to add function

create_criteria_df which takes tuples ("criteria_name", [criteria vector]) to create a dataframe of crteria with "criteria_name"'s as columns.
First arguments will be mandatory criteria (I'm thinking coral cover, connectivity and heat/wave stress but we can leave out any of these)
Pseudo code:
function crteria_df(cover, space, connectivity, heat_stress,wave_stress, criteria...)              criteria_df = DataFrame(coral_cover=cover, coral_space = space, connectivity=connectivity, heat_stress=heat_stress,                                   wave_stress=wave_stress)             for each crit in criteria                       criteria_df[!,crit[1]] = crit[2] end return criteria_df

Also add functions

create_tolerance_df which takes tuples ("criteria name", [tol_val, "gt/lt"]), where "criteria_name" corresponds to any column names in criteria_df. Creates a dataframe with the same columns as criteria_df, but the first row is tolerance values and the second indicates filtering is greater than or less than with "gt" or "lt". Takes any number of inputs (and will check for name matches to criteria_df).
create_weights_df which takes tuples ("criteria_name", weight) with all "criteria_name"'s matching columns of criteria_df.
When ARIA initialises mcda variables, create_tolerance_df, create_weight_df and create_criteria_df can be called once and then just updated at each time step.

Additional parameters for distance sorting can be stored in a structure to be passed into guided_site_selection, such as the distances matrix, min_dist etc.

New  create_decision_matrix function then simply converts criteria_df to an array and applies filters specified in the tolerances dataframe.
Psuedo code:

create_decision_matrix(criteria_df,tolerances_df)

A = Matrix(criteria_df)
crit_cols = names(criteria_df)
tol_cols = names(tolerances)

    for name_ind in 1:length(tol_cols)
        find index of name in crit_cols
        if found
            if  "gt" in tolerance_df at index
                set rule <= tolerances_df value at index
            if "lt"  in tolerance_df at index
                 set rule > tolerances_df value at index
            end
            A[findall(rule), index] .= NaN
        end
    end

filtered = vec(.!any(isnan.(A), dims=2))
    # remove rows with NaNs
    A = A[filtered, :]
return A
end

Could you add julia to the code blocks so the markdown engine knows which language so that we get syntax highlighting?
Just add julia after the initial three backticks
```julia

create_seed_matrix and create_shade_matrix will become a single function create_intervention_matrix, which takes A, criteria_df, weights_df and a vector of strings indicating the criteria to be used for the particular intervention. I imagine these as sim_constants maybe.
For exmaple the seeding matrix `SE = create_intervention_matrix(A,criteria_df, weights_df, ["coral_space", "connectivity","heat_stress","wave_stress"])
Psuedo code :

function create_intervention_matrix(A,criteria_df, weights_df,intervention_criteria_names)
     Find indices of cols in criteria_df corresponding to names in intervention_criteria_names
     Select these cols from A (+site_id column)
     Find indices in weights_df corresponding to names in intervention_criteria_names
     Select weights from weights_df using indices and normalise
     return intervention matrix, normalised weights
end

In ADRIA, seeding and shading matrices (and fogging if we choose to separate fogging and shading) can be created using create_intervention_matrix with different vectors of strings indicating criteria to be included in the matrix.
If/when we switch to using the julia package for mcda, the same functions can be used to make the decision matrices and weightings used.

JMcDM is implemented as

w  = [0.35, 0.15, 0.25, 0.20, 0.05];
fns = [minimum, maximum, minimum, minimum, maximum];
result = topsis(Matrix(df), w, fns);

So the matrix and weights can still be used with the package.

Could the comments above be collated into a single comment per related set of changes, as opposed to a series of small comments? It's hard to track what is related to what at the moment.
I'll review once you're done writing things out.

Change 1 - New set of functions for creating dataframes defining weights, tolerances and criteria:

Plan to add function create_criteria_df which takes tuples ("criteria_name", [criteria vector]) to create a dataframe of crteria with "criteria_name"'s as columns.
First arguments will be mandatory criteria (I'm thinking coral cover, connectivity and heat/wave stress but we can leave out any of these)
Pseudo code:

 function crteria_df(cover, space, connectivity, heat_stress,wave_stress, criteria...)

       criteria_df = DataFrame(coral_cover=cover, coral_space = space, connectivity=connectivity, heat_stress=heat_stress,
       wave_stress=wave_stress)
       for each crit in criteria
             criteria_df[!,crit[1]] = crit[2]
      end
      return criteria_df
end

criteria names would ideally be assigned by converting the input argument name to a string, but I couldn't find a way to do this with varargs (if you know of one?)
create_tolerance_df which takes tuples ("criteria name", [tol_val, "gt/lt"]), where "criteria_name" corresponds to any column names in criteria_df. Creates a dataframe with the same columns as criteria_df, but the first row is tolerance values and the second indicates filtering is greater than or less than with "gt" or "lt". Takes any number of inputs (and will check for name matches to criteria_df).
create_weights_df which takes tuples ("criteria_name", weight) with all "criteria_name"'s matching columns of criteria_df.
When ARIA initialises mcda variables, create_tolerance_df, create_weight_df and create_criteria_df can be called once and then just updated at each time step.
Additional parameters for distance sorting can be stored in a structure to be passed into guided_site_selection, such as the distances matrix, min_dist etc.

Change 2 - create_decision_matrix will have a more generic structure and will use tolerances_df to filter a matrix created from criteria df. create_seed_matrix and create_shade_matrix can be folded into a single function create_intervention_matrix which selects columns and weights for the intervention of interest.

New create_decision_matrix function simply converts criteria_df to an array and applies filters specified in the tolerances dataframe.
Psuedo code:

function create_decision_matrix(criteria_df,tolerances_df)

      A = Matrix(criteria_df)
      crit_cols = names(criteria_df)
      tol_cols = names(tolerances)

    for name_ind in 1:length(tol_cols)
        find index of name in crit_cols
        if found
              if  "gt" in tolerance_df at index
                  set rule <= tolerances_df value at index
              if "lt"  in tolerance_df at index
                  set rule > tolerances_df value at index
              end
             A[findall(rule), index] .= NaN
         end
     end

     filtered = vec(.!any(isnan.(A), dims=2))
     # remove rows with NaNs
     A = A[filtered, :]
     return A
end

Returned matrix A can then be used in current MCDA algorithms, or can be converted to df for use with suggested mcda package.

there's probably a better way to indicate whether the tolderance is greater than or less than but that's the basic idea.

create_seed_matrix and create_shade_matrix will become a single function create_intervention_matrix, which takes A, criteria_df, weights_df and a vector of strings indicating the criteria to be used for the particular intervention. I imagine these as sim_constants maybe.

For exmaple the seeding matrix `SE = create_intervention_matrix(A,criteria_df, weights_df, ["coral_space", "connectivity","heat_stress","wave_stress"])

Psuedo code :

function create_intervention_matrix(A,criteria_df, weights_df,intervention_criteria_names)
     Find indices of cols in criteria_df corresponding to names in intervention_criteria_names
     Select these cols from A (+site_id column)
     Find indices in weights_df corresponding to names in intervention_criteria_names
     Select weights from weights_df using indices and normalise
     return intervention matrix, normalised weights
end

In ADRIA, seeding and shading matrices (and fogging if we choose to separate fogging and shading) can be created using create_intervention_matrix with different vectors of strings indicating criteria to be included in the matrix.
If/when we switch to using the julia package for mcda, the same functions can be used to make the decision matrices and weightings used.
mcda is implemented in JMcDM as

w  = [0.35, 0.15, 0.25, 0.20, 0.05];
fns = [minimum, maximum, minimum, minimum, maximum];
result = topsis(Matrix(df), w, fns);

So the matrix and weights can still be used with the package, with perhaps guided being a string instead of an integer indicating the algorithm to use.

Change 3- Move calculation of criteria into set of functions to be called prior to initialising and updating criteria_df.

adds a new file to sites, perhaps mcda_io.jl or similar which has functions which perform required calculations to produce criteria as vectors of length nsites to input in create_criteria_df
For example, the heat stress and wave stress criteria could be created with

function env_stress_criteria(env_stress)
    return maximum(env_stress) != 0.0 ? (env_stress .- minimum(env_stress)) ./ (maximum(env_stress) - minimum(env_stress)) : zeros(Float64, size(env_stress))
end

An initialisation function initialise_mcda() would take in all necessary arguments to call these criteria functions and input to criteria_df.
An updating function update_mcda() would call these criteria functions to create updated criteri vectors to add to criteria_df.
This takes any reference to specific criteria formulations outside of the mcda functions so they can stay generic.

Thank you, this is much nicer. Could you delete the other comments now that we have this write up?
As a general rule, in dynamic languages like Julia/Python/R/MATLAB - and especially in languages that support functional programming concepts - if you find that you're relying on a lot of loops, string matching and (nested) if/else statements, it is usually an indication that the design or envisioned approach is overly complicated and needs to be rethought. They are design smells so to speak.
Have a read through my responses to "Change 1" first and see if it might impact/change the design of the other changes.
Response to Change 1:
If the arguments for crteria_df() are used to construct a DataFrame, then are there any adverse implications from:

using splatted keyword arguments?
using a NamedDimsArray+AxisKeys instead of a DataFrame (better performance)

function criteria_df(; criteria...)
    # Add a check that the expected criteria are provided.
    # Ideally this would be agnostic to the order of inputs
    # (what if we need to add another kind of connectivity later? etc etc)
    return DataFrame(criteria)
end

If we're planning to leverage JMcDM, and it expects a matrix as the argument anyway, then we can avoid transforming between DataFrames -> matrices and just use matrices with named indices.
Note that readable code with documentation removes the need for you to specify the return type in the function name. The _df suffix, I don't find it particularly helpful, could a more informative name be thought up?

create_tolerance_df which takes tuples ("criteria name", [tol_val, "gt/lt"]), where "criteria_name" corresponds to any column names in criteria_df. Creates a dataframe with the same columns as criteria_df, but the first row is tolerance values and the second indicates filtering is greater than or less than with "gt" or "lt". Takes any number of inputs (and will check for name matches to criteria_df).

Is the plan here is to parse the string and run the associated operator?
I understand why associating the threshold with the criteria is necessary, but this approach is very complicated.
Strings are also computationally expensive to parse over and over (this is why Julia uses/recommends Symbols for the most part).
I would association the comparison function itself (note that operators are generic functions in Julia).
julia> <
< (generic function with 142 methods)

julia> <(5, 6)
true

I think the argument formatted as a tuple of (name, vector{value, string}) is going to cause issues - it's not straightforward.
It could just be a function with splatted keyword args (as above).
function criteria_tolerances(; thresholds...)
    # return named matrix of lambdas
end

criteria_tolerances(; cover=(0.5, <), heat=(0.3, >), ... so on and so forth ...)

The return could be a named vector of lambdas/anonymous functions, of size N (where N == number of criteria).
The lambda would be something like x -> <(x, threshold_val)
Each entry has the criteria name as its key, and its entry is the comparison function.
As a for loop, it could be used like (I've not tried this of course):
for thresh in criteria
    thresh(val)
end

But you don't need a for loop in this case, you could just broadcast the values.
See: this comment
map.(criteria, some_values)

# Or using pipe syntax

some_values .|> criteria

create_weights_df which takes tuples ("criteria_name", weight) with all "criteria_name"'s matching columns of criteria_df.

Use named indices and you don't need this.

Additional parameters for distance sorting can be stored in a structure to be passed into guided_site_selection, such as the distances matrix, min_dist etc.

Is this a current or new struct?
Would this need to be updated each run? What performance implications are there?
For Change 2:
As mentioned above, seems your planned changes rely on a lot of string matching for loops and nested if statements.
Please consider the feedback above and have a think about how you would simplify things.

So the matrix and weights can still be used with the package, with perhaps guided being a string instead of an integer indicating the algorithm to use.

A limitation of the samplers we're using is that they only work with numeric values.
So it has to be a (discrete) value that can be mapped to an associated function. We can map the value directly to the function of interest, no need for a string.
See under "Examples" - function can be selected from a list of available methods.
https://jbytecode.github.io/JMcDM/dev/utility/#mcdm
An implication is if new methods are added to JMcDM which changes the order of that list, messing up our expected sampled value -> function mapping.
But this can be handled by only updating JMcDM between "minor" versions of Julia.
For Change 3:

For example, the heat stress and wave stress criteria could be created with

Is this just min-max normalization?
Is it that different criteria require different normalization approaches?
Do we really need a criteria-specific function to do this?
Do all criteria require a normalization step?
We already have an ADRIA-context specific normalization function. Can we use that instead?
https://github.com/open-AIMS/ADRIA.jl/blob/9dc45df4c53271aa83046e65bb85c2f7d2b223a6/src/analysis/analysis.jl#L43

adds a new file to sites, perhaps mcda_io.jl or similar which has functions which perform required calculations to produce criteria as vectors of length nsites to input in create_criteria_df

What are the "required calculations"? An example would be appreciated.
Generally io would indicate it is related to file interaction or networks or something along those lines.
Are you thinking of reading/writing data as part of the process here?

An updating function update_mcda() would call these criteria functions to create updated criteri vectors to add to criteria_df.

Sounds like this would allocate memory? Have you thought of separating the core behaviour into a separate function so that it can be used to update matrices in place?

In response to your comments:

Happy to work with NamedDimsArrays andAxisKeys instead of DataFrames
I wasn't aware you could do that with julia operators so happy to do that for the tolerances
I will of course attempt to limit loops where possible
In your comment about criteria specific functions I probably used the wrong example. Many of the criteria are much more complicated to create than the example I used and so it is messy to do these inline without a function. For example creating the zones criteria is L457-L474 here :   https://github.com/open-AIMS/ADRIA.jl/blob/9dc45df4c53271aa83046e65bb85c2f7d2b223a6/src/sites/dMCDA.jl#L457
The creation of the priority predecessor criteria is similarly several lines.  I also think maybe this is what you mean here:  "Have you thought of separating the core behaviour into a separate function so that it can be used to update matrices in place?"

For the additional considerations/feature requests:

We need to be able to constrain site selection to a specific spatial area (or areas). I have a very hacky approach for the recent ADRIA-informed ReefMod runs to work, but this kind of change should also be considered with the changes above
You might as well address https://github.com/open-AIMS/ADRIA.jl/issues/231 while you're at this

Could this be done using an extnesion to the depth_priority index, where only the indices of the set of sites in the specified area are used?

Other models use data not currently considered. These include CoTS connectivity, Cyclone severity where values are cyclone categories 1 - 5 (I think, will confirm). Will discrete values affect anything?

Discrete values won't effect anything :)

Once you have the return from MCDA - what do you do with it? Does anything change on that front?
The return is slightly different for the JMcDM functions but not too different, so just some tweaks here and there.

Are the outputs conceptually identical to what we're using now? Yes

What implications are there for ADRIA (both in terms of design and code)?
I don't think it will drastically change ADRIA proper, but depending on the format of the outputs of JMcDM the seeding and shading ranks logging may change.

Happy to work with NamedDimsArrays andAxisKeys instead of DataFrames

Reminder to please use it in a way that is consistent with how its used elsewhere in ADRIA. e.g., Always use NamedDimsArray over KeyedArray (the two are not equivalent).

I wasn't aware you could do that with julia operators so happy to do that for the tolerances

You can do this in most languages. The syntax will be different of course.

In your comment about criteria specific functions I probably used the wrong example.

My objection wasn't about the use of a function per se, it was about the need to manually code up the function call for each criteria.
How do you know which criteria is more complicated, and are there common steps that can be done beforehand?
For example, in the example you linked above, it still normalizes the values. Is this a common step that should be done beforehand?

Could this be done using an extension to the depth_priority index, where only the indices of the set of sites in the specified area are used?

I guess so - I've again hacked something so it works with ReefMod data for now - involves an extra argument to the run_site_selection function.
On this note - we should move away from having "sites" in our function names as we're trying to be far more generic and "sites" is an overloaded term.
Something like ADRIA.select_locations() is short and understandable I think. Do you have any suggestions?

depending on the format of the outputs of JMcDM the seeding and shading ranks logging may change

Okay, please have a close look at JMcDM - including trying to use it by itself - and see if the needed changes are worth it.
I'd also ask that you profile its performance. No point in doing all this work to incorporate it if it means ADRIA runs will take minutes instead of milliseconds.

Additionally, it may make sense to make the guided_site_selection generic to an intervention type - so we don't have to keep adding things if we want different interventions, we just change the criteria matrix and the rest stays the same.

Yes, that's one of the aims with this rewrite.

criteria_tolerances(; cover=(0.5, <), heat=(0.3, >), ... so on and so forth ...)

Realised this would look neater if you swap the elements:
criteria_tolerances(; cover=(<, 0.5), heat=(>, 0.3), ... so on and so forth ...)

This way the order neatly implies x < 0.5

Some notes from looking into comparing JMcDM and the functions we have currrently for the mcda.
Comparing the average runtime for performing site rankings for 2000 randomised decision matrices with randomised weightings, 9 criteria and 200 sites, the current mcda functions were 3-4X faster when comparing Topsis and almost 8X faster when comparing Vikor.
Rankings are the same for the Topsis algorithm, as expected, but slightly different for Vikor. This is probably because there are many versions of Vikor which have further steps to detrermine trade-offs in the final set of ranks etc,  so the package probably uses a variation.
Comparing a set of 1024 scenarios using the mcda topsis and JMcDM topsis (without distance sorting), there was no significant difference in absolute coral cover:

Comparing a set of 1024 scenarios using the mcda vikor and JMcDM vikor (without distance sorting), the JMcDM game higher coral cover in the middle of the time period on average:

From this it seems there is some benefit for more complicated techniques like vikor, but no real benefit for less complicated techniques like topsis.
(In both figures light blue lines are summed difference in absolute coral cover over sites, dark blue is the average over scenarios)

Hi Rose, how were the timings taken? Did you use BenchmarkTools?

I used time(), is it preferable to use BenchmarkTools?

For certain things yes - I was more interested in how you collected timings.
time() as in you took the start time then subtracted it from the end time? Like tic/toc in MATLAB?
Or do you mean the @time macro?

Yeah so I did time_start = time() and then dt = time()-time_start

The average time difference per run varied quite a bit depending on the number of runs for JMcDM but the mcda functions were always at least a little bit faster.

Okay, I think it's okay but note that at a minimum the @time macro is preferred for performance profiling.
Calculating the time elapsed that way will give misleading results because of the JIT compilation process. Given you're doing it for each run and taking the average it's less of an issue I think but in future use @time instead, and do it twice at a minimum.

It's a good idea to first run the code you intend to profile at least once (unless you want to profile Julia's JIT-compiler)

https://docs.julialang.org/en/v1/manual/profile/#Basic-usage
On the whole I guess I'm not surprised that JMcDM would be a little slower - likely they're doing extra checks or some other process we're not doing. Did you look at the code to see what the difference is?

I seems to be from the average timings, but I can check again using @time and running the code befroe timing to avoid compile time.
I've looked into topsis and they use a loop for calculating scores, while we use matrix operations. I think they do this because they allow the user to have different objectives for each column (whereas we transform each column so it is always maximised), but it's not entirely clear.
I just checked the vikor code and they are using the same formulation as us but they use a nested loop where they loop through each criteria and alternative and then have if statements to check whether you are maximising or minimising. So I think that's probably why it's slower, plus you have to set up a MCDM object for each run which can't be set up ouutside of the time loop because it includes the decision matrix.

if statements to check whether you are maximising or minimising. So I think that's probably why it's slower

Okay, so it's slower because of the additional flexibility. They could just prepare a vector of functions and apply that for each row/column as we discussed (remember, our conversation about how operators are functions - they could do the same but create purpose-specific functions for each row/column as needed - and do that once instead of checking every time in a loop).
Given what you found, what would you prefer - stick with our own or adopt JMcDM?
You could always submit a PR to JMcDM as well to address this performance issue.

Despite it being quite a bit slower, it could be cool to have access to their larger range of functions and be able to explore different site selection methods. What do you think?

Following yesterdays meeting, it was decided to add Criteria structures particular to each model in use to specify the criteria which can be used in site selection. For example, ReefMod will have additional values for the weights of cots for seeding and shading :
Base.@kwdef struct ReefModCriteria{P,N} <: EcoModel
wave_stress::P = Param(1.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Wave Stress", description="Importance of avoiding wave stress. Higher values places more weight on areas with low wave stress.")
    heat_stress::P = Param(1.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Heat Stress", description="Importance of avoiding heat stress. Higher values places more weight on areas with low heat stress.")
    in_connectivity_shade::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Shade Connectivity", description="Higher values give preference to locations with high connectivity for shading deployments.")
    in_connectivity_seed::P = Param(1.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Incoming Connectivity (Seed)", description="Higher values give preference to locations with high incoming connectivity (i.e., receives larvae from other sites) for enhanced coral deployments.")
    out_connectivity_seed::P = Param(1.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Outgoing Connectivity (Seed)", description="Higher values give preference to locations with high outgoing connectivity (i.e., provides larvae to other sites) for enhanced coral deployments.")
    coral_space_seed::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Low Coral Cover", description="Higher values give greater preference to sites with low coral cover for seeding deployments.")
    coral_cover_shade::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="High Coral Cover", description="Higher values give preference to sites with high coral cover for shading deployments.")
    priority_seed::P = Param(1.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Predecessor Priority (Seed)", description="Importance of seeding sites that provide larvae to priority reefs.")
    priority_shade::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Predecessor Priority (Shade)", description="Importance of shading sites that provide larvae to priority reefs.")
    zone_seed::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Zone Predecessor (Seed)", description="Importance of seeding sites that provide larvae to priority (target) zones.")
    zone_shade::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Zone Predecessor (Shade)", description="Importance of shading sites that provide larvae to priority (target) zones.")
cots_seed::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="COTs (Seed)", description="Importance of seeding sites that are COTs management sites.")
cots_shade::P = Param(0.0, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="COTs (Shade)", description="Importance of shading sites that are COTs management sites.")heat_stress_tol::P = Param(0.2, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Low Area Tolerance", description="Tolerance for low proportional space for seeding deployments.")
wave_stress_tol::P = Param(0.2, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Low Area Tolerance", description="Tolerance for low proportional space for seeding deployments.")
    coral_cover_tol::P = Param(1.0, ptype="real", bounds=(0.75, 1.0), dists="unif",
        name="Risk Tolerance", description="Filters out sites with heat/wave stress above threshold.")
    use_dist::N = Param(1, ptype="integer", bounds=(0.0, 1.0 + 1.0), dists="unif",
        name="Use Distance Threshold", description="Turns distance sorting on or off.")
    dist_thresh::P = Param(0.1, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Distance Threshold", description="Sites selected by MCDA must be further apart than median(dist)-dist_thresh*median(dist).")
    top_n::N = Param(10, ptype="integer", bounds=(5.0, 50.0 + 1.0), dists="unif",
        name="Top N", description="Replaces a given deployment site with a top-ranked site if it does not satisfy the minimum distance threshold.")
    depth_min::P = Param(5.0, ptype="real", bounds=(3.0, 5.0), dists="unif",
        name="Minimum Depth", description="Minimum depth for a site to be included for consideration.\nNote: This value will be replaced with the shallowest depth value found if all sites are found to be deeper than `depth_min + depth_offset`.")
    depth_offset::P = Param(10.0, ptype="real", bounds=(10.0, 25.0), dists="unif", name="Depth Offset",
        description="Offset from minimum depth, used to indicate maximum depth.")
end

In the case of site selection without running the ADRIA ecological model, the Domain will include the criteria with keys matching those in the criteria dataframe structure. For example, if using the criteria type above, the key for in-coming connectivity would be 'in_connectivity'. The appended '_seed', '_shade' or '_tol' will indicate the parameter's usage.
In site_selection, the KeyedArray holding criteria can then be created by functions similar to those below:

function create_criteria_store(site_ids::AbstractArray; criteria...)

    criteria_matrix = site_ids
    for crit_key in keys(criteria)
        criteria_matrix = hcat(criteria_matrix, criteria[crit_key][site_ids])
    end
    return KeyedArray(criteria_matrix[:, 2:end], reefs=site_ids, criteria=collect(keys(criteria)))

end

function create_criteria_store(site_ids::AbstractArray, domain::Domain, criteria_df::DataFrame)
    criteria_names = collect(criteria_df.columns)

    criteria_names = replace.(replace.(criteria_names, "_seed" => ""), "_shade" => "")
    criteria_names = Symbol.(unique(criteria_names[!occursin.("tol", criteria_names)]))
    criteria_values = Tuple(getproperty(domain, criteria_n) for criteria_n in criteria_names)
    criteria = (; zip(criteria_names, criteria_values)...)

    return create_criteria_store(site_ids; criteria)
end

create_criteria_store takes the column names of the criteria dataframe row (which is created from sampling using ReefModCriteria), and uses these to select out the criteria from the domain to create a keyed array which will be used to construct decision matrices.
Inside site_selection, the criteria_store is created and input directly into guided_site_selection, which will create the decision matrix using criteria_store, and tolerances, a KeyedArray we mentioned above which maps criteria to filtered versions based on a rule:
function create_decision_matrix(criteria_store::KeyedArray, tolerances::KeyedArray)

    for tol_key in tolerances.tols
        rule = map(tolerances(tol_key), criteria_store(tol_key))
        criteria_store = criteria_store[rule, :]
    end

    return criteria_store
end

There would be a constructor function to create tolerances from the criteria dataframe (tolerance values would be identified as criteria_name+"_tol"), but I'm not sure how best to input the > and < specifications. Maybe this can just be an input into site_selection().
Decision matrices for particular interventions and their weightings will then be created from the criteria_store, criteria_df and the decision matrix created from criteria_store (A) :
function create_intervention_matrix(A::Matrix, criteria_df::DataFrame, criteria_store_keys::Tuple, intervention::String)
    # Get intervention criteria names
    int_crit_names = names(criteria_df)[occursin.(intervention, names(criteria_df))]
   # remove intervention name to index criteria store
    int_crit_names_store = replace.(int_crit_names,string("_",intervention)=>"")
    int_ind = [findall(criteria_store_keys.==Symbol(criteria_name)) for criteria_name in int_crit_names_store]

    # get correct columns from A
    S = A[:, int_ind]
     # get correct weights from criteria_df
    ws = normalize(Array(criteria_df[int_crit_names]))
    return S, ws
end
function create_seed_matrix(A::Matrix, criteria_df::DataFrame)
    return create_intervention_matrix(A, criteria_df, "seed")
end
function create_shade_matrix(A::Matrix, criteria_df::DataFrame)
    return create_intervention_matrix(A, criteria_df, "shade")
end

The "seed" or "shade" beside each criteria name in the criteria dataframe is used to identify the indices in A to use for the intervention matrix and the weights in criteria_df to use. The rest of guided_site_selection would be the same, with the possibility of using the matrix A and weights in a JMcDM method or one of the local mcda methods. Additionally, because we are passing in the criteria_df row, this will contain dist_thresh and min_dist, so we won't need these to be in mcda_vars and can get rid of the mcda_vars structure.
The main issue I can see with this is criteria need to be in the form they will be used in in the criteria matrix when they are stored in the Domain structure (when it is created). Because of this there might need to be separate Domains for when we are using standalone site_selection with ADRIA data vs the ecological model. But maybe we are unlikely to use standalone site selection for ADRIA without the ecological model now that we have other model datasets available?
Another issue is how to deal with heat stress, wave_stress and other time dependent layers. Do we treat heat_stress and wave_stress as separate, as we do now, and store all time steps in the domain and then select out the time step we want? This will mean that heat stress and wave stress are special cases because they cannot be put directly into the criteria_store constructor through the domain. Or do we only store the timestep we want in the domain? This will again require different domain structures for if we are doing standalone site selection with ADRIA or the full ecological model.
I recognise there's a lot of string operations which isn't great, but I reckon I can simplify what I have above by maybe finding the indices corresponding to seeding and shading in the criteria_df and criteria_store initially and passing these where needed (but also let me know if you have a better alternative).
Let me know what you think and if any of that made sense :)

Thanks Rose, it's looking clearer. My comments below.
We discussed not using DataFrames for performance reasons.
As discussed above, please always use NamedDimsArray instead of KeyedArray.
The two are functionally equivalent, but are of different types as far as Julia is concerned, and given we already use NamedDimsArray (and we have functions that expect that type), use that instead so ADRIA code is consistent.

if using the criteria type above, the key for in-coming connectivity would be 'in_connectivity'. The appended '_seed', '_shade' or '_tol' will indicate the parameter's usage.

criteria_names = replace.(replace.(criteria_names, "_seed" => ""), "_shade" => "")
criteria_names = Symbol.(unique(criteria_names[!occursin.("tol", criteria_names)]))

This will not be generic enough for our purposes. You're requiring us to go back and edit this line every time a different kind of intervention is added. Please see my comment at the end.
This is already a known issue right now (see #277).
Also, replace() already handles multiple conditions.
julia> x = "egg egg2 egg3"
"egg egg2 egg3"

julia> replace(x, "2"=>"A", "3"=>"B")
"egg eggA eggB"

For the create_intervention_matrix (fourth code block)
function create_shade_matrix(A::Matrix, criteria_df::DataFrame)
    return create_intervention_matrix(A, criteria_df, "shade")
end

Same issue as above really, this could be made much more generic - currently you'd have to come back and add another function (and its use) for every intervention have or decide to include in the future.
To resolve, you could change Domain to hold MCDA related data in a separate struct that includes metadata and select the relevant data set using that instead.
An approach that relies on string parsing is to adopt a standardized naming pattern and identify any/all relevant criteria types based on that. For example (taking a leaf from ReefMod naming conventions):
"iv__conn__seed"
"iv__conn__shade"
Here, "iv" indicates an intervention.
Note the double underscore to distinguish from a single underscore typically used to indicate a space.
Then match on "iv__" (excluding tol as you have above).
This is not a great approach but would "work" - ideally the Domain and MCDA-related types would work seamlessly together without having to rely on string comparisons.

Thanks Takuya.
My main reason for using the dataframe is to attempt to replace the use of mcda_vars with just the sampled criteria weights dataframe (in the case of standalone site selection), or param_set in the case of ADRIA the ecological model. I wanted to do this because most of the construction of mcda_vars is just reassigning elements of param_set or the sampled criteria dataframe.
A way around having to refer to particular intervention methods is perhaps then to separate out seed criteria and shade criteria prior to create_intervention_matrix(), so the input "criteria_df" (which can be changed to be a NamedDimsArray) only has criteria which will be used in seeding or shading. We need something which indicates which criteria are used in either intervention and so how I was going to do this was using the "seed" or "shade" at the end of criteria column names in the sampled dataframe (or param_set). Perhaps instead we could indicate this in the Domain? Maybe have an "interventions" variable. Which contains a Tuple of symbols indicating the criteria to be used for each intervention. The name of each tuple could indicate the intervention name (so that fieldnames(Domain.interventions) gives a list of the intervention names) which could be used to find the corresponding weights in the sampled criteria dataframe or param_set. What do you think?

I think what you want to do is separate out the Criteria type
https://github.com/open-AIMS/ADRIA.jl/blob/77d8b3c664d59b0ba424aaaf3947987b6eaae292/src/ecosystem/Ecosystem.jl#L76

I wanted to do this because most of the construction of mcda_vars is just reassigning elements of param_set or the sampled criteria dataframe.

Yes, hence why I don't think the mcda_vars is necessary.

Do you mean have a different Criteria type for different intervention types?

It's not just the intervention criteria is it?
In any case you would define Criteria to be an abstract type, and then rename the current "Criteria" to something else, and add a new type, such that
SomeGenericCriteria <: Criteria
InterventionCriteria <: Criteria

Does that make sense?

In your example above, would you have
SomeGenericCriteria <: Criteria
SeedCriteria <: Criteria
ShadeCriteria<:Criteria

Or am I misunderstanding...

Doesn't that mean you have to specify each intervention type yourself?

I'm just not sure how to map weightings to criteria. So for example in the current criteria we have weightings for seeding and shading for the same criteria (e.g in_connectivity_seed, in_connectivity_shade) and then we have some which are unique to seeding and shading (coral cover and space for coral, which also have their own weightings). All of the weightings are in the same structure generated by sampling Criteria(), so we need some way of identifying which intervention they site under. If we use criteria, we have to do some text matching to work out which are for which intervention, but this does require naming the intervention type in the criteria in some way, which is not very generic...

Okay so we could extend the metadata attached to each criteria Param.
This would require modifying the functions which collate all the parameters (model_spec() is one there may be other places) to ignore this Criteria-specific metadata.

Okay cool, so with iv_type, could we put a tuple, like iv_type=(:seed,:shade) (for example, because wave_stress is used for both seeding and shading, and then add intervention types if we need?

hmmm, okay that's not going to work as neatly as I thought (because you'd have to go and specify it yourself) but might be the best we can do for now.
I think it should be okay because we've agreed that at a minimum we'd write model-specific Criteria sub-types anyway.

Ok cool, we can work with this for now and if we think of something better we can change.

Actually, sorry just thought of something. If we have, iv_type=(:seed,:shade), can we still make sure when sampling that two values are sampled (one for seeding and one for shading) or is that not possible? If we do it this way I might have to rewrite the site_selection sampler so it can sample for a specified set of interventions (so if you want weights for seeding and shading, it knows to sample 2 values for each parameter with iv_type = (:seed,:shade). Is that getting too complicated?

If we have, iv_type=(:seed,:shade), can we still make sure when sampling that two values are sampled (one for seeding and one for shading) or is that not possible?

I think I've confused myself.
In the example above:
wave_stress_tol::P = Param(0.2, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="Low Area Tolerance", description="Tolerance for low proportional space for seeding deployments.",
        iv_type=(:seed, :shade)
)

Are you saying if you were not to adopt this, you'd specify this twice? Something like:
seed_wave_stress_tol::P = ...
shade_wave_stress_tol::P = ...

Yes, so either way you have a specification of which are seeding or shading, but in the first case it's not in the variable name...

Yeah I think the above approach is definitely preferrable, but just pointing out that we'll have to adjust the sampling process because I think at the moment it creates one instance of each parameter for each scenario. With the new approach, in the case of , for example,in_seed_connectivity and shade_connectivity, these are weights for the same criteria just for seeding and shading. So we would define
in_connectivity::P = Param(0.2, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="In connectivity ", description="Weighting for in coming connectivity criteria.",
        iv_type=(:seed, :shade)
)

And then 2 samples will be pulled for the one parameter per scenario, one for weighting in_connectivity while seeding and one for when shading. The same would happen for zone_shade and zone_seed and seed_priority and shade_priority. Unless we want to reduce the complexity and make the weightings the same for different interventions, but this may not be desirable for some interventions.

What I meant was you'd specify the two anyway:
# except make the names/description more informative of course
in_seed_connectivity::P = Param(0.2, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="In connectivity ", description="Weighting for in coming connectivity criteria.",
        iv_type=:seed
)
in_shade_connectivity::P = Param(0.2, ptype="real", bounds=(0.0, 1.0), dists="unif",
        name="In connectivity ", description="Weighting for in coming connectivity criteria.",
        iv_type=:shade
)

Ah I see, ok cool, that makes things simpler.

Please see my edit above

Sorry, could you expand on what you mean by match on the type fieldname(in_shade_connectivity) ?

in_shade_connectivity was an example of a fieldname.
struct Example
   A  # <- this is a field, hence the fieldname is "A"
end

Suggest you look up the documentation for fieldname() and getfield() will give you ideas on how to write functions to help access specific fields cleanly.

I know that it's a fieldname and you can use getfield to access a field of a particular name from a structure, but do you mean using this in place of iv_field?

Yes, that's what I meant by "Perhaps a more straightforward approach is to match on the type fieldname"
You could use iv_type, but this way you don't have to change anything else though as I mentioned earlier.
In case you were wondering, it's different from the example you had above as you're not relying on a dataframe of names.
However, this snippet here ...
criteria_names = replace.(replace.(criteria_names, "_seed" => ""), "_shade" => "")
criteria_names = Symbol.(unique(criteria_names[!occursin.("tol", criteria_names)]))
criteria_values = Tuple(getproperty(domain, criteria_n) for criteria_n in criteria_names)
criteria = (; zip(criteria_names, criteria_values)...)

... would mean conn_seed and conn_shade both resolve to conn. Was this intended?

That's because there's only one vector of in_connectivity values for each site (which is used to construct decision matrices), but it has 2 weights (in_connectivity_seed and in_connectivity_shade). So the idea was to use the "seed" and "shade" identifiers later to construct the weights vectors, but for the criteria matrix you don't use the seed and shade identifiers because you have overlap.
But with getfield I still don't quite understand how you'd do it. Can you use getfield with a wildcard, like getfield(struct,*seed)? The code above is used to identify the fields in Domain which contain criteria, as these don't have the same names as the weights in Criteria(). I thought if we have a consistent naming convention we'd be able to match to two, but there's still the issue that Criteria() doesn't map one-to-one with the criteria in Domain.

Can you use getfield with a wildcard, like getfield(struct,*seed)?

You would define your own if this was the approach you wanted to take but I'd prefer to match against known identifiers.
This is because something may change later (e.g., we add seed_field_that_should_not_be_match) and we get erroneous matches without noticing.

The code above is used to identify the fields in Domain which contain criteria, as these don't have the same names as the weights in Criteria()

The purpose of using a consistent naming scheme is to side-step that issue. Is this related to the discussion around the MCDA_vars type?

Even with a consistent naming scheme though, you need something which tells you which names are being used as criteria. If we get the list of names as fields from Criteria which contain "seed" or "shade", then to access them in domain we'd have to use the name without "seed" or "shade" in it. Unless we store the same criteria vectors under different names with seed and shade in them, but that seems like a waste.

Even with a consistent naming scheme though, you need something which tells you which names are being used as criteria.

You mean like iv__*?

so I would write something that can use getfield with a wildcard, like getfield(Domain,*iv__) ?

If you need all "intervention" related fields, that's the idea yes. Doesn't have to be a type-specific getfield either, that was just a off-the-cuff suggestion. Just a function would suffice here too.
Or you could use the iv_type idea discussed above and find all parameters with the matching entry for that.

Ok cool, thanks I think I get it now :)

Hey @ConnectedSystems, for your ReefModDomain, do you create a new constrcutor function, or does the old constructor work ?
Just wondering because if I add criteria to say ReefModDomain or ADRIADomain then I'll have to add different constructors, which seems complicated...

Has to be a new constructor.

Just wondering because if I add criteria to say ReefModDomain or ADRIADomain then I'll have to add different constructors, which seems complicated...

Could you elaborate on this a little?

Right, ok. Just because there's 2 functions which work together to construct the domain, and it seems like I'll have to re-write variations of both of them to fill the intervention criteria keys in the domain structure. Just wanted to check there wasn't an easy way around this.

It sounds like what I suggested earlier would address this perhaps, but requires the structure of Domain to change too.
In fact, I think that's preferable because the way things are at the moment is really messy and ugly.
Instead of this:
https://github.com/open-AIMS/ADRIA.jl/blob/2f926ada0c9487f0a18e878c9d0ffe9eabff1e5e/src/Domain.jl#L209-L211
You'd have:
Domain(name, rcp, env_layer_md, ..., some_type_name_that_indicates_relation_to_MCDA_and_also_holds_the_info_you_want, ...)

Then it's clear whats relevant for the MCDA.

Oh - it doesn't have to be struct/type, probably a namedtuple is what you're after.

For example though, if one of the new Domain variables is iv__priority or iv_zones, which require about 4 or more lines to be calculated, should these be calculated in the constructor or left as placeholders which can be filled later?

Wait hang on - could you give an example of what those are for?
Is there a need for them to be part of Domain or was it just a placeholder/holdover from MATLAB?

These are criteria which are eventually part of the criteria matrix for mcda. iv__priority is the priority predecessors criteria and iv__zones is the priority zones criteria. I just thought they'd be stored in the Domain because we talked about storing all criteria in the domain for standalone site selection.

I just thought they'd be stored in the Domain because we talked about storing all criteria in the domain for standalone site selection.

The thinking there was that the relevant parameters have to at least be accessible via the Domain.
But specifically the ones you mention here, are they calculated just once and used throughout or are they to be calculated before selection/model runs?
What I'm trying to determine is how Domain.some_mcda_parameter_set would work and how appropriate it is to shove everything in there if they're going to be recalculated or thrown away anyway.

Yeah they're just calculated once but the calculation is about 4 lines for iv__priority and longer for iv_zones. Should the ones which are recalculated, like coral cover,  be put as placeholders in Domain and then added when needed?

So maybe in the standalone process we include variables for each of the criteria (which won't have to be recalculated because there's no time dependence) and then in the ecological model we maybe just construct the NamedDimsArray of criteria first and then update this- does that sound reasonable?

Hey, a question regarding how you want to use run_site_selection with ReefMod - do you need (or generally see the need in potential future usage) to be able to have different initial coral covers for a given number of criteria samples and domain? We currently have this but as I'm making changes I thought I'd see if there's anything we're unlikely to need...

Just tagging you @ConnectedSystems (and no rush to read because I know you're working on that presentation this afternoon), because I'm floating some better formats for the output of site_selection and run_site selection (currently unintuitive and difficult to use when aggregating data).
I'm considering changing the outputs to (no. of scenarios, no. of sites, 2) and instead of ranks as the entries we have the site names in order from highest ranked to lowest ranked, following the same syntax as keys for the NamedDimsArrays holding results such as coral cover etc. Sites which are filtered out will be replaced with a placeholder (maybe NaN or whatever you think is best) so that the size of each scenario set is consistent. These scenario vectors of site names could then be used to directly index results arrays by discarding placeholder values.

do you need (or generally see the need in potential future usage) to be able to have different initial coral covers for a given number of criteria samples and domain?

Yes, potentially.

Sites which are filtered out will be replaced with a placeholder (maybe NaN or whatever you think is best) so that the size of each scenario set is consistent.

We were using $n_{sites} + 1$ to indicate locations which were not considered, but this wasn't consistent.

I'm considering changing the outputs to (no. of scenarios, no. of sites, 2) and instead of ranks as the entries we have the site names in order from highest ranked to lowest ranked, following the same syntax as keys for the NamedDimsArrays holding results such as coral cover etc.

I can't quite visualize what's meant here so bear with me. If the current structure is $\text{scenarios} \cdot \text{sites} \cdot \text{[seeding, shading]}$ (where the final dimension is to be expanded to three to explicitly indicate fogging vs SRM) then what would the new structure be? $\text{ranks} \cdot \text{scenarios} \cdot [\text{seeding, fogging, SRM}]$ ?

Ok cool, I'll keep the potential for multiple coral covers in- I'll do this by creating the criteria storage structure initially from the domain and then replacing the coral_cover which is loaded from the domain for each new site selection scenario (in the case of run_site_selection).
Also, I'm using the most current version of run_site_selection from Main (through a recent rebase), and there is an additional site filter so you end up with considered_sites being your final vector of site_ids to be considered in site_selection. Is this something added for ReefMod? I'm currently testing with ADRIA data and had to comment it out for testing because I got empty site_id vectors. What is the correct way to use the considered_sites variable?

I can't quite visualize what's meant here so bear with me. If the current structure is scenarios⋅sites⋅[seeding, shading] (where the final dimension is to be expanded to three to explicitly indicate fogging vs SRM) then what would the new structure be? ranks⋅scenarios⋅[seeding, fogging, SRM] ?

Yeah, so you might have an output that looks something like this for 3 scenarios (I've only shown the seeding dimension):
3-dimensional NamedDimsArray(KeyedArray(...)) with keys:
↓   scenarios ∈ 8-element UnitRange{Int64}
→   ranks ∈ 216-element Vector{String}
◪   intervention ∈ 3-element Vector{String}
And data, 8×216×3 Array{Float64, 3}:
[:, :, 1] ~ (:, :, "seeding"):
           (rank 1)                        (rank 2)                         (rank 3)                             (rank 216)
 (1)   "Briggs_BR_C_1"       "Briggs_BR_OF_1"         "Briggs_BR_OF_2"      ...       "Thetford_TR_S_7"
 (2)    "Briggs_BR_OF_1"      "Briggs_BR_C_1"           "Briggs_BR_OF_2"      ...                 NaN
 (3)     "Briggs_BR_OF_1"     "Briggs_BR_OF_2"            "Briggs_BR_C_1"     ...                 NaN

[:, :, 2] ~ (:, :, "fogging"):
...

I've used NaN here to indicate that some sites were filtered, so there's a smaller set of ordered sites than 216.  Each row is a scenario containing site ids ordered according to how they were ranked. So the top 3 sites in the first scenario were [ "Briggs_BR_C_1"       "Briggs_BR_OF_1"         "Briggs_BR_OF_2"     ].  Does that kind of make sense?

Is this something added for ReefMod?

Yes - see usage below. The reason was that although ReefMod simulates the GBR, for the specific simulations we were working with we only wanted to deploy within specific regions.
From memory I ran into issues with an empty location list but this was due to a different reason somewhere in the MCDA process (can't remember what). You should do a comparison between the "ADRIA-ReefMod" branch and "Main" because there's a catch in run_site_selection() to consider all available locations if the list of target locations are empty.
This was a tentative implementation (read: I needed something to work quickly) and definitely needs to be cleaned up. I dislike having a this potentially infinite number of optional keyword arguments (and yes I know I used "sites" again - these should be "locations", like I said, rush job).
# Target priority sites
# - must be in the small reef size class
# - must be in the Cairns region
# - must have > 0.01 km² of `k` area
subset_of_sites = findall((dom.site_data.size_group .== 1) .& (dom.site_data.cairns_region .== true) .& ((ADRIA.site_k_area(dom) .* 1e-6) .> 0.01))
@info "Selecting from $(length(subset_of_sites)) locations"
ranks = ADRIA.run_site_selection(dom, scens, sum_cover, area_to_seed, ts; target_seed_sites=subset_of_sites')

Is this something added for ReefMod?

Yes - see usage below. The reason was that although ReefMod simulates the GBR, for the specific simulations we were working with we only wanted to deploy within specific regions.
From memory I ran into issues with an empty location list but this was due to a different reason somewhere in the MCDA process (can't remember what). You should do a comparison between the "ADRIA-ReefMod" branch and "Main" because there's a catch in run_site_selection() to consider all available locations if the list of target locations are empty.
This was a tentative implementation (read: I needed something to work quickly) and definitely needs to be cleaned up. I dislike having a this potentially infinite number of optional keyword arguments (and yes I know I used "sites" again - these should be "locations", like I said, rush job).
# Target priority sites
# - must be in the small reef size class
# - must be in the Cairns region
# - must have > 0.01 km² of `k` area
subset_of_sites = findall((dom.site_data.size_group .== 1) .& (dom.site_data.cairns_region .== true) .& ((ADRIA.site_k_area(dom) .* 1e-6) .> 0.01))
@info "Selecting from $(length(subset_of_sites)) locations"
ranks = ADRIA.run_site_selection(dom, scens, sum_cover, area_to_seed, ts; target_seed_sites=subset_of_sites')

Ok cool, thanks. I'll check out "ADRIA-ReefMod" to understand it and integrate it with what I have. I also need to go through my code and make sure I haven't accidentally referred to sites instaed of locations anywhere :'D.

My thoughts were because they are the site ids or indices in order of their rank you can use them directly to index a result set. So if you wanted to access coral cover for the top 10 ranked sites you just take the site ids for the top 10 directly.

What would top 10 mean though? If they run 5 different scenarios with 5 different top 10s, what would the next step be?

Top 10 ranks. So you could compare the coral cover at the top 10 ranked sites for each scenario. You could also use this to calculate the frequencies of selection for different sites.

This goes back to the "gymnastics" comment though. Are you envisioning the user does all this by hand or are you thinking of a report function that provides those stats?

I can write a function that does that, from your comment it just seemed like you wanted changes to the output format of site_selection. But I can do that.

Well keep in mind this issue and discussion therein: https://github.com/open-AIMS/ADRIA.jl/issues/324

Well I thought it would be better to have the site orderings as raw output so there's flexibility and then have a series of aggregation functions which can be used if further analysis is needed, like a site frequencies one etc

Hey @ConnectedSystems, just an update on using a Dirichlet distribution for the weighting parameters. I had a better look into the parameterisation of the distribution (it's been a while since I've used one). The parameter it uses, $\alpha$, is a concentration parameter, so it determines whether each variable in the multivariate distribution has a homogenous or non-homogeneous spread. To get an effectively uniform distribution across each variable we'd use $\alpha_i=1 \forall i=1,...,n_var$, where $n_var$ is the number of weights. This should be farily simple to set up, so if you're happy I can start to implement it.
https://en.wikipedia.org/wiki/Dirichlet_distribution

Wait sorry, how will this be implemented?
If it's simply changing the distribution of the weights, you just have to modify the Params entry for the Criteria type.

It's a little bit more complicated because its a multivariate distribution, so you have an $\alpha$ for each of the weights (it will be 1 for a uniform marginal dist) but the sample is drawn as a vector which will sum to one. I'm thinking maybe we have dists="dirichlet" and then alpha=1 instead of "bounds" in Ecosystem.  Then we add some extra lines in sample() for dists = "dirichlet", which gathers all alpha values for variables with this type of distribution and then draws a sample vector for each realisation. Happy to leave this for a separate issue though if you'd prefer?

Yes, I say leave it for now. Sobol' is a deterministic sampling approach and mixing this in with it will destroy the sampling scheme.
I'm also unsure how this addresses the normalization issue so I'll have to read up on it.

Ok cool, I'll leave it. I realised it's also a little more complicated than what I wrote above because we'd have to draw 1 vector for the seed weights and 1 for the shade weightsa, etc, which is difficult because we have overlap in these parameters (e.g. the heat stess and wave stress weights are both used for seeding and shading, which would break the independence). There's a way of drawing the sample based on Gamma distributions so you reduce it to a univariate distribution problem and then normalise with a value drawn from the Gamma distribution with alpha value as the sum of the other gamma distribution's alpha values... but yeah more complicated than I first thought.
Should I write up an issue?

Please go ahead and raise an issue.

Hey @ConnectedSystems, so I just read through our previous comments to work out what I've addressed and what I haven't and we mentioned maybe making the site selection functions generic to intervention. This is quite built in at the moment so I thought I would detail how I think it could be done before going ahead. At the moment guided_site_selection (now called guided_location_selection) takes in prefseedsites, prefshadesites, logseed and logshade. I'm thinking that as we already use logseed and logshade in scenario to decide when to seed and shade, we remove these as inputs in guided_site_selection and put calls to guided_site_selection inside these shading/seeding if statements. Instead of prefseedsites and prefshadesites inside guided_site_selection, we just have prefsites and an additional string which indicates the intervention type (which will match the ends of the weights names in Criteria{}). We then call guided_site_selection twice if we are choosing sites for seeding and shading separately in scenario and once if we are using the same sites for seeding and shading. What do you think?

Note that you need to separate fogging from shading as well
I think we need to keep the process to consider all interventions together so the decisions are made as holistic as possible. Particularly important when we consider economics: we want to maximize coverage (spread the deployment locations out) while balancing costs (the further spread out locations are, the most deployments will cost). I'm not sure yet if ships/boats used for deployment can do all interventions (shade/fog/seed) or if a ship is configured for only one kind of deployment only.

Regarding the code itself, please remember to update variable names to snake_case as you come across older naming styles.

Ah yes, I'll look into separating fogging and shading.
What do you mean by the process to consider all interventions together? Currently the seeding and shading decisions are either evaluated separately or the same set of intervention sites is used for seeding and shading (decided using the seeding matrix). Do you mean the fact that very similar decision matrices are used for seeding and shading? If so, we are definitely keeping this.

What do you mean by the process to consider all interventions together?

Sorry, getting ahead of myself. What I was thinking was that depending on how the ships are configured, it may be that we have to consider the deployment locations for seeding/shading/fogging all together. For example, if a single ship can fog and seed then we'd want fog/seed locations to be somewhat in close proximity to reduce costs. I don't know if this is actually the case, just an option we'd want to keep open.

Right, I see, thanks. One way of incorporating this is allowing multiple interventions to be considered in one decision matrix, so for example criteria tagged as 'shade' and 'fog' are all used in the same matrix which is solved for ranks. In terms of 'close by' sites instead of sites that satisfy criteria for both interventions, you could probably select for one intervention first (such as seeding) and then maybe do distance sorting of the sites for the second intervention based on their distance from the sites selected for the first... but this would take some alterations to the distance_sorting function

Hey @ConnectedSystems , I'm in the process of implementing shading separately to fogging. I'm wondering if we need a separate variable to n_site_int which designates how many clusters to shade. For example the Moore set has 5 clusters, so you'd just shade everything if you select the 5 best clusters to shade. Should I add a new variable or should we start simple and assume you only shade one cluster? (so you pick the highest ranked in the mcda)...

Start simple and just re-use n_site_int please. For Moore it makes sense to shade everything because it's a small area.

I'll do that, thanks

Hey @ConnectedSystems , do we want to keep track of the shading ranks like with seeding and fogging?  If so, it makes things a bit complicated because the size of ranks is the number of sites not the number of clusters. I could just store in the upper part of the matrix but then the site ids wouldn't correspond to anything. I'm trying to avoid adding a special case so it's easier adding interventions in the future...

Flagging this PR is ready for review, but it's probably best to resolve #329 fist so I can correct the definition of connectivity in the mcda criteria.

Closing this issue as I think this has become stale or otherwise superseded by other work.


Hard-coded &str to constants

A string that is currently commonly used in several files is hard-coded.
e.g.
Eq, Output, Self etc.
It should be convert these to constants as they may cause typos.
like
const EQ: &str = "Eq";

Hi, I'm interested in this. Can I take this issue?

Of course.
If you have any questions, please ask here or in discord.

For example, mono("Num") appears 8 times in erg_compiler/context/initialize/mod.rs. This can be changed to, for example
let Num = mono("Num");

Please put them in one place.

Sure, I'll try that! Thank you for the swift response and elaborate explanation!

Ah, I'm sorry I referred this issue as "Fixed" from the pull request, so this issue seems to be closed at the same time 😅 Can you reopen this? @mtshiba
Thanks!


syncer: improve mutation + admission

syncer: move secret renaming to mutation
Signed-off-by: Steve Kuznetsov<EMAIL_ADDRESS>
syncer: add an interface for mutation
Signed-off-by: Steve Kuznetsov<EMAIL_ADDRESS>
syncer: move configmap mutation into mutators
Signed-off-by: Steve Kuznetsov<EMAIL_ADDRESS>
syncer: move serviceaccount mutation to mutators
Signed-off-by: Steve Kuznetsov<EMAIL_ADDRESS>
syncer: add a concept for admission
Signed-off-by: Steve Kuznetsov<EMAIL_ADDRESS>
/assign @jmprusi @ncdc

This PR IMO over-abstracts both the mutator interface and the admission as a separate concept. It adds complexity that won't help us.

/close

::no-prow::


[DIALServer] Fixes to align the DIALServer with the DIALSpec (2.2)

For this purpose a new flag has been added, "runtimechange" on AppSection to diffrentiate between apps that can change the URL after being started (WebKit -> Result 200) and apps that con not (Cobalt -> Result 501)

Looks good to me.

https://jenkins.code.rdkcentral.com/job/github-triggered/1103/ started


Supporting Cline requests style.

Feature request / 功能建议
Cline requests are quite different in the content section which might be a list instead of str.
{
	"model": "qwen-2.5-coder-instruct",
	"messages": [{
		"role": "system",
		"content": "You are Cline, a highly skilled software engineer with extensive knowledge in many programming languages, frameworks, design patterns, and best practices......"
	}, {
		"role": "user",
		"content": [{
			"type": "text",
			"text": "<task>......</task>"
		}, {
			"type": "text",
			"text": "<environment_details>.....</environment_details>"
		}, {
			"type": "text",
			"text": "[TASK RESUMPTION] ....."
		}, {
			"type": "text",
			"text": "<environment_details>.......</environment_details>"
		}, {
			"type": "text",
			"text": "[TASK RESUMPTION] ......"
		}, {
			"type": "text",
			"text": "<environment_details>......</environment_details>"
		}]
	}],
	"temperature": 0,
	"stream": true,
	"stream_options": {
		"include_usage": true
	}
}

So the default chat_template might cause the following error:
TypeError: [address=<IP_ADDRESS>:33357, pid=544] can only concatenate str (not "list") to str

Motivation / 动机
It would be great that Cline or other similar tools can use Xinference's API.
Similar issues happened in DeepSeek API, and seems fixed. see https://github.com/cline/cline/issues/230 .

But welcome to contribute on this feature.

Cline is a VS code extension, previous named Claude Dev.
I think it would be a lot of work since every LLM model has it's own chat_template. Do you think custom chat_template is a solution?

Does Cline has its own API router like openai ones /v1, if so, we can tell the message is from Cline, and we can write a function to convert the messages.

Cline supports API providers like OpenRouter, Anthropic, OpenAI, Google Gemini, AWS Bedrock, Azure, and GCP Vertex. You can also configure any OpenAI compatible API, or use a local model through LM Studio/Ollama. If you're using OpenRouter, the extension fetches their latest model list, allowing you to use the newest models as soon as they're available.

Cline is design to work with Anthropic Claude 3.5 Sonnet, it should be OpenAI API compatible. I think it just a client that construct requests and then send to API. It's hard to tell whether the request is from Cline. The request constructed does not contain any information says it's from Cline.

That would a bit more nasty, we have to analyze the content of messages, if it contains the structure like the example, we convert it to flattened normal openai messages.

That would a bit more nasty, we have to analyze the content of messages, if it contains the structure like the example, we convert it to flattened normal openai messages.

liteLLM is using this strategy.
https://github.com/BerriAI/litellm/blob/fd583e715ec1b183cade08ac29c1cbe73e5c94f6/litellm/llms/mistral/mistral_chat_transformation.py#L170
https://github.com/BerriAI/litellm/blob/fd583e715ec1b183cade08ac29c1cbe73e5c94f6/litellm/litellm_core_utils/prompt_templates/common_utils.py#L27
https://github.com/BerriAI/litellm/blob/fd583e715ec1b183cade08ac29c1cbe73e5c94f6/litellm/litellm_core_utils/prompt_templates/common_utils.py#L63

Yeah, it costs some, a simple way is just pass the messages to models, if TypeError raised, we then check and covert, IMO, that would be better.

If the requests content are all list, handling errors will also cost extra resources. Even worse than the first strategy.

OK, for messages we do the conversion I think it's OK.

@gahoo Do you have interest to contribute this feature?

I have tried two methods, and the test results are both okay.
Method 1: Modify the chat-template to add the following content:

Method 2: Add a static method _handle_messages_with_content_list_to_str_conversion in the ChatModelMixin class in llm/utils.py to implement message conversion, as detailed below:
    @staticmethod
    def _handle_messages_with_content_list_to_str_conversion( messages: List[Dict]) -> List[Dict]:
        """
        Handles messages with content list conversion
        """
        for message in messages:
            texts = ""
            msg_content = message.get("content")
            if msg_content:
                if isinstance(msg_content, str):
                    texts = msg_content
                elif isinstance(msg_content, list):
                    for c in msg_content:
                        text_content = c.get("text")
                        if text_content:
                            texts += text_content
            if texts:
                message["content"] = texts
        return messages

Additionally, modify the async_chat method in the VLLMChatModel class to call _handle_messages_with_content_list_to_str_conversion, as detailed below:
    @vllm_check
    async def async_chat(
        self,
        messages: List[Dict],
        generate_config: Optional[Dict] = None,
        request_id: Optional[str] = None,
    ) -> Union[ChatCompletion, AsyncGenerator[ChatCompletionChunk, None]]:
        messages = self._handle_messages_with_content_list_to_str_conversion(messages)
        tools = generate_config.pop("tools", []) if generate_config else None
        ... ...

Thanks @hwzhuhao , nice work, I prefer the method 2 which is more general. Willing to hear your opnions.

I also agree to use method 2. The chat-template is usually provided by the model provider, so it is not suitable for modifications.

I also agree to use method 2. The chat-template is usually provided by the model provider, so it is not suitable for modifications.

I modified the code in the Docker image using method 2 above, but it didn't work

The xinf version I tested locally is 1.0.1, and the vllm version is 0.6.1.post2. Could you show which version are you using?

The xinf version I tested locally is 1.0.1, and the vllm version is 0.6.1.post2. Could you show which version are you using?

Hey guys, welcome for you to submit a PR to support Cline style request, we can let others to benefit the features.


PineconeError: PineconeClient: Error calling upsert: TypeError: stream.getReader is not a function

Even though I am copy pasting the codes from the Langchain documentation just to understand the flow but still I am getting this error.

It was my mistake only, I just updated the node version from 16 to 18 and it worked.


Issue integration Envoy tracing with Spring Sleuth

Have problem integrating Envoy tracing with Spring Sleuth
Description:
We are using Envoy with Jaeger and zipkin tracer.
e.g.
tracing:
      http:
        name: envoy.zipkin
        config:
          collector_cluster: jaeger
          collector_endpoint: "/api/v1/spans"

Looks like that egress traffic's spanid always get rewritten to something like 1,2,3,4. It is ok when manually forwarding headers inside the service by preserving the ingress spanid.
But when propagating tracing headers with Spring Sleuth and zipkin tracer, looks like sleuth always creates new spanid for the egress traffic as well.
So in the jaeger ui, there is warning saying invalid parent span IDs=xxxxxxxxx; skipping clock skew adjustment makes
a -> b -> c -> d

becoming
a -> b
c -> d

My questions are:

How to integrate with systems that has its own tracer defined?
Why envoy/jaeger always rewrites the egress spanid & parentspanid?

Thanks !!!

Are you sending the spans, created by Spring Sleuth, also to the zipkin service provided by Jaeger?
Can you provide some examples of the traces (screen shots initially), both when doing manual context propagation and with sleuth?

@objectiser yes, we are sharing the same jaeger collector endpoint for traces with and without sleuth.
When we are testing it, it always returns 404 for both curl http:jaeger-collector.ns.svc.cluster.local:9411/api/v1/services and  curl http:jaeger-collector.ns.svc.cluster.local:9411/api/v2/services. is this the correct way?


GIFs/Videos not loading sometimes

I don't know how to fix this, help wanted

I really am new to JS myself ... I was troubleshooting Videos playing fine and audio on those videos being really slow ... thought it was Umatrix (Addon) https://github.com/gorhill/uMatrix or one of its settings or filters misbehaving but after I saw this issue only affected Firefox and not in the new layout (when GoodTwitter is disabled), I just pinpoint it to be something in the addon doing it ...I will take a look to see if I can see what's happening  but there are TONS of people that may be able to fix this and do it much faster than I could

Should be fixed in #23