Repository: pydicom/deid (language: python)

Issue:
action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?

action order on same field
Hello,
I am confused by following in [docs](https://pydicom.github.io/deid/user-docs/recipe-headers):

> For the above, given that there are conflicting commands, the more conservative is given preference. For example:
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
> ...
> If I specify to blank a header and remove it, it will be removed
> If I specify to replace a header and blank it, it will be blanked

Also

> Most of the time, you won’t need to specify remove, because it is the default

But at the same time there is an example of
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
Here I naturally make assumption that `ALL` means `ALL`, so each field is removed, and according to rules quoted above, `REMOVE` rules over `KEEP`, so tag kinda has to be removed.
How does it work? How is `PixelData` expected to be kept?

In my usecase I want to drop all information except for certain tags, part of them I want to keep, part of them I want to replace with my custom func.
I don't want to manage full set of DICOM tags, but rather remove all and preserve only certain subset.
How?



Discussion / hints:
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 
I would try to follow the example and do a REMOVE ALL and then add the fields you want to keep. If that is successful we can update the docs to reflect it - I think the spirit of the docs is that if you were to have all those commands in the same file for the same field, it's saying that `REMOVE` would take preference. But it also could be that they are out of date.

Please test it out for your use case and report back!
I tested on 
```
REMOVE ALL
KEEP SeriesDescription
```
and it ends up with everything removed. Same if I swap lines.
So seems like if I run `REMOVE ALL`, I can't `REPLACE` or `KEEP` any tag anymore.

So no idea how can I only concentrate on tags I need with dropping everything that I don't need with one-liner (aka whitelisting)?

Writing documentation for open-source project based on blackbox testing sounds really weird. 
ah then the documentation is correct. :)

> Writing documentation for open-source project based on blackbox testing sounds really weird.

I don't understand this comment (it seems like it could be a bit insulting) but I'll just say that if you see an issue in the docs you are welcome to suggest changes. In this case the docs are correct, so there's that.

In practice it's not hugely practical to just wipe every header field from a dicom - many of them are needed. I would suggest you look at your data and come up with reasonable filters (e.g., removing everything with a date, or name, the library supports regular expression matching for that).

Insulting the maintainers of the library for their documentation is not going to get you very far. Just FYI. :)
I'm sorry it it sounded insulting, just in my world documentation is based on actual code logic and is updated together with code changes, otherwise other teams depending on product will struggle. It is not exactly an open-source world, but I am trying to follow same practices when contributing into OSS :)

As for this specific example in docs:
```
ADD PatientIdentityRemoved Yes
REMOVE ALL
KEEP PixelData
```
which doesn't make too much sense (even just-added `PatientIdentityRemoved` is removed and not present in outcome), I can only suggest separate paragraph about `REMOVE ALL` behavior and dropping example.

Sadly my usecase is exactly about dropping everything except for tags needed to display image, and uuids required to distinguish between studies are being anonymized. The rule we got from healthtech guys from google is "use whitelists to avoid GDPR/etc problems".  In total it makes about ~30 fields to be left in file. In one of example recipes I found there is 240 rows of tags being replaced - I just don't want to manage such a big list, if I could go with ~30 lines whitelist. 

What do you think if I i.e. raise PR with whitelisting support? Not sure yet, how, if I would like to keep existing `REMOVE ALL` behavior for backwards compatibility. Some kind of switch, or totally new keyword like `BLACKLIST ALL` which can be overridden for tags by following regular actions?
So that order would become
> REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD > BLACKLIST

meaning that tag is removed unless any of existing actions is added to field (existing actions become whitelisting-actions).

Smth like that. Would it fit into deid roadmap?
No worries @glebsts - sometimes it's hard to tell with communication on these boards.

Your use case is new, and I think we could definitely support it with the right tweaks, and it would also be good to fix that (now dated and not working) section of the docs. 

Note that we do have a "protected" set of fields https://github.com/pydicom/deid/blob/master/deid/dicom/config.json that would hurt the integrity of the dicom file if removed, so if you see something not being removed that you would expect, take a look there.

We have whitelisting for the filter section of deid, so logically we could support some kind of the same thing for header fields. But I'm wondering can we handle this case just with the current tags? E.g., if REMOVE ALL and then a series of KEEP were to work, wouldn't that be sufficient? I ask because it would be messy in the format of the deid.dicom to have a header with both action statements and filters, especially when the action statements can already have regular expressions.

Let me think more about it - and feel free to write up a spec in the meantime. I'm going into a few hours of work and ping me again if I don't remember to respond!
Oh and could you give me an example (with comments) about how a blacklist/whitelist action would work? 
I myself go to sleep now, it is midnight here in GMT+3 DTS :) Will get back to OSS in couple of days.
`REPLACE ALL + KEEP` to work would make totally sense, but I was thinking about keeping backwards compat. That would be a breaking change, and if set into major release, can be done as is. My `BLACKLIST` suggestion was only because of backwards compatibility. If it is not important, could just normalize behavior of replace+(keep|add|jitter|replace) combo. 
That would just mean changing order to 
> BLANK > REPLACE > JITTER/KEEP/ADD > REMOVE

But I was not yet too deep in deid internals, not sure if I can first iterate whole recipe and build list of fields to keep and check it upon every remove action. Will see..
To answer your first question - yes we would absolutely be open to a PR to make this change - looking forward to chatting in a few days when you are back!
Hi there, quite long read for you - tests and my three statements and suggestions I'd like to implement. Vanessa, pls add your opinion on those before I start digging in code.
___
So I added some tests to `test_replace_identifiers` that illustrate minimal logic points that I'd like to clarify.
___
```
    def test_remove_all_keep_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertEquals('20230101', parser.dicom['StudyDate'])
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error KeyError: (0008, 0020)` (which is `StudyDate`)
___
```
    def test_remove_except_field_keep_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "except:Manufacturer"},
            {"action": "KEEP", "field": "StudyDate"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)

        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertIsNotNone(parser.dicom['Manufacturer'])
        self.assertIsNotNone(parser.dicom['ManufacturerModelName'])
        self.assertIsNotNone(parser.dicom['StudyDate'])
```

that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0070) Manufacturer                        LO: 'SIEMENS'
(0008, 1090) Manufacturer's Model Name           LO: 'SOMATOM Definition AS+'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `Error self.assertIsNotNone(parser.dicom['StudyDate'])`

___
```
    def test_remove_all_add_other_field_compounding(self):
        dicom_file = get_file(self.dataset)

        actions = [
            {"action": "REMOVE", "field": "ALL"},
            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
        ]
        recipe = create_recipe(actions)

        parser = DicomParser(dicom_file, recipe=recipe)
        print(f"Before: {parser.dicom}")
        parser.parse()

        print(f"After: {parser.dicom}")

        self.assertEqual("Yes", parser.dicom['PatientIdentityRemoved'].value)
        self.assertIsNotNone(parser.dicom['PixelData'])
        self.assertNotEqual("19700101", parser.dicom['StudyDate'].value)
```
that produces following:
```
Before: Dataset.file_meta -------------------------------
# ... skipped lots of fields here
(0008, 0020) Study Date                          DA: '20230101'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements

After: Dataset.file_meta -------------------------------
# [File Meta Information Group Length, File Meta Information Version, Transfer Syntax UID, Implementation Class UID]
# skipped here, they are preserved from original data
(0008, 0020) Study Date                          DA: '19700101'
(0012, 0062) Patient Identity Removed            CS: 'Yes'
(7fe0, 0010) Pixel Data                          OB: Array of 9778 elements
```
**and fails** with `AssertionError: '19700101' == '19700101'`
___

Here I have following statements (order not related to tests):
1) `ADD tag` is working with `REMOVE`, adding new tag after everything has been dropped, even same tag. Not exactly behavior of `REMOVE > ADD` rule in case of existing tag (kinda mimics `REPLACE`), but it is a behavior I would actually expect when reading the recipe. **I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing `ADD` from `REPLACE > ..... > KEEP/ADD` rule.**
1) `KEEP tag` is not working with `REMOVE tag`, `REMOVE except:tag` or `REMOVE ALL`, and if I want to keep `StudyDate`, I can't do it by whitelisting it.
I assume semantically `REMOVE ALL` followed by `KEEP tag` is expected to keep `tag`. It doesn't. **I'd like to change `deid` to support such flow.**
2) `REMOVE tag`/`REMOVE except:tag` is working with substrings (didn't add test above, but if you make `REMOVE except:Model`, it keeps `ManufacturerModelName`. I find this place slightly unclear in docs, `except` section of [docs](https://pydicom.github.io/deid/user-docs/recipe-headers/) says "list of field", while it is actually regex per se. There are examples with explicit `contains:Name` also in [docs](https://pydicom.github.io/deid/examples/header-expanders/), but lib acts as it is implicit. **I'd like to clarify that by updating `except` documentation, though might miss it in some other place.**


Thank you for the detailed write-up - here are my thoughts so you can move forward:

> ADD tag is working with REMOVE, adding new tag after everything has been dropped, even same tag. Not exactly behavior of REMOVE > ADD rule in case of existing tag (kinda mimics REPLACE), but it is a behavior I would actually expect when reading the recipe. I am not sure if I want to change behavior, because it works for me, but I would clarify that in docs by removing ADD from REPLACE > ..... > KEEP/ADD rule.

I agree - I would remove the REMOVE > ADD tag from the docs, as I believe it reflects the original implementation of deid where we parse over actions first, decided on some "final" state and then proceeded. This version moves through the rules top to bottom and essentially follows them, so it makes sense the user can ADD, REMOVE, ADD, REMOVE in infinitude. It indeed does mimic replace, at least with that sequence of actions. I don't think we should change this behavior, but should just fix that section in the docs.

> KEEP tag is not working with REMOVE tag, REMOVE except:tag or REMOVE ALL, and if I want to keep StudyDate, I can't do it by whitelisting it.

If ADD/REMOVE can be thought of as changing with state, KEEP would be akin to a whitelist, as to say "no matter what happens, do not remove this tag." And that probably means it is allowed to be edited (e.g., KEEP would work with REPLACE or ADD but just not remove). The easiest way to implement this would be to parse KEEP first, and then ensure if something is to be REMOVE it is not in the KEEP set.

> I assume semantically REMOVE ALL followed by KEEP tag is expected to keep tag. It doesn't. I'd like to change deid to support such flow.

I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

> REMOVE tag/REMOVE except:tag is working with substrings (didn't add test above, but if you make REMOVE except:Model, it keeps ManufacturerModelName. I find this place slightly unclear in docs, except section of docs says "list of field", while it is actually regex per se. There are examples with explicit contains:Name also in docs, but lib acts as it is implicit. I'd like to clarify that by updating except documentation, though might miss it in some other place.

I would absolutely love an improvement to the docs here! I likely documented the pattern to use a regex but not in context of a particular tag.
Thank you for comments.
> I agree, and probably in the docs we would want to stress that most actions are done based on current state, but keep is more global (and thus doesn't matter where it's used).

Could you please explain what do you mean by "based on current state"?

In general I take that as "Yes" and will proceed with implementing whitelists (aka "KEEP overrules REMOVE") and updating docs.
> Could you please explain what do you mean by "based on current state"?

Sure! So imagine that we have a box with different colored balls (this is the dicom file, and each ball is a header entry). If we then have a list of actions, we can move down the list one by one and interact with the balls. E.g.,:


- REMOVE red   # the new state is that the box has no red balls (state 1)
- ADD red           # the box has a red ball again (state 2)
- REMOVE red # again they are gone, we are back to state 1

So those are stateful actions because using them at some timepoint will change the state of what is in the box. So KEEP would be a global command (not stateful) so if you did the same:

- KEEP red   # a global statement that says "freeze the state of the box so we always keep red balls, I don't care what stateful actions come after)
- REMOVE red           # we don't remove any red, we have the same state
- ADD red # again, no change

So (just generally) when I'm comparing actions like ADD/REMOVE I'm thinking of those as stateful because they are scoped to a specific point in time. A KEEP, however, is not specific to a point and time - no matter where you use it in your recipe it is going to "pin" some global state. Does that kind of make sense?

Yes, thank you, nice example :) I'll think how to put it into words without using balls, but saying in advance - feel free to patch my wording, as you seem to be native speaker :)
Haha definitely! It's probably not the best example - they always used [colored balls in bags as examples in statistics courses](https://thefactfactor.com/facts/pure_science/mathematics/statistics-and-probability/getting-a-red-ball/15167/), so I'm not sure I did it justice :laughing: 

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3871c627..02ba1751 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ and **Merged pull requests**. Critical items to know are:
 Referenced versions in headers are tagged on Github, in parentheses are for pypi.
 
 ## [vxx](https://github.com/pydicom/deid/tree/master) (master)
+ - `ADD/KEEP` actions have now higher priority than `KEEP`, allowing to whitelist fields from `REMOVE ALL` [#197](https://github.com/pydicom/deid/issues/197)
  - updated pydicom dependency from 2.1.1 to 2.2.2 [#194](https://github.com/pydicom/deid/issues/194)
  - various LGTM alert fixes [#186](https://github.com/pydicom/deid/pull/186)
  - bug fix for exception when attempting to jitter DA/DT which cannot be jittered (space) [#189] (https://github.com/pydicom/deid/issues/189) (0.2.27)
diff --git a/deid/dicom/parser.py b/deid/dicom/parser.py
index 0e55bbbb..4e3c68bf 100644
--- a/deid/dicom/parser.py
+++ b/deid/dicom/parser.py
@@ -269,6 +269,20 @@ def skip(self):
             skips = self.config.get("get", {}).get("skip", {})
         return skips
 
+    @property
+    def keep(self):
+        """
+        Return a list of fields to keep, as defined by all KEEP actions in recipe
+        """
+        keeps = []
+        if self.recipe.deid is not None:
+            keeps = [
+                action.get("field")
+                for action in self.recipe.get_actions(action="KEEP")
+                if action and action.get("field")
+            ]
+        return keeps
+
     def get_fields(self, expand_sequences=True):
         """expand all dicom fields into a list, where each entry is
         a DicomField. If we find a sequence, we unwrap it and
@@ -279,7 +293,7 @@ def get_fields(self, expand_sequences=True):
                 dicom=self.dicom,
                 expand_sequences=expand_sequences,
                 seen=self.seen,
-                skip=self.skip,
+                skip=self.skip + self.keep,
             )
         return self.fields
 
@@ -321,18 +335,18 @@ def find_by_name(self, name):
 
     # Actions
 
-    def perform_action(self, field, value, action, filemeta=False):
+    def perform_action(self, field, value, action):
         """perform action takes an action (dictionary with field, action, value)
         and performs the action on the loaded dicom.
 
         Parameters
         ==========
-        fields: if provided, a filtered list of fields for expand
+        field: a field for expand
+        value: field value
         action: the action from the parsed deid to take
            "field" (eg, PatientID) the header field to process
            "action" (eg, REPLACE) what to do with the field
            "value": if needed, the field from the response to replace with
-        filemeta (bool) perform on filemeta
         """
         # Validate the action
         if action not in valid_actions:
@@ -344,7 +358,7 @@ def perform_action(self, field, value, action, filemeta=False):
             values = self.lookup.get(re.sub("^values:", "", field), [])
             fields = self.find_by_values(values=values)
 
-        # A fields list is used vertabim
+        # A fields list is used verbatim
         # In expand_field_expression below, the stripped_tag is being passed in to field.  At this point,
         # expanders for %fields lists have already been processed and each of the contenders is an
         # identified, unique field.  It is important to use stripped_tag at this point instead of
diff --git a/deid/version.py b/deid/version.py
index 682d58c4..1b9c44e2 100644
--- a/deid/version.py
+++ b/deid/version.py
@@ -22,7 +22,7 @@
 
 """
 
-__version__ = "0.2.28"
+__version__ = "0.2.29rc1"
 AUTHOR = "Vanessa Sochat"
 AUTHOR_EMAIL = "vsochat@stanford.edu"
 NAME = "deid"
diff --git a/docs/_docs/user-docs/recipe-headers.md b/docs/_docs/user-docs/recipe-headers.md
index ad2d567e..7bdbaaae 100644
--- a/docs/_docs/user-docs/recipe-headers.md
+++ b/docs/_docs/user-docs/recipe-headers.md
@@ -36,7 +36,7 @@ The application does the following, by default, taking a conservative de-identif
 
 However, you might want to do either of the following:
 
- - have a specific action for some set of headers, where actions include `BLANK`, `REPLACE`, `JITTER`, `REMOVE`, and `KEEP`
+ - have a specific action for some set of headers, where actions include `BLANK`, `REPLACE`, `JITTER`, `REMOVE`, `ADD` and `KEEP`
  - perform some custom functions between `get`, `update`, and `put`.
 
 We will show you a working example of the above as you continue this walkthrough. For now, let's review configuration settings. 
@@ -221,16 +221,16 @@ be taken on a header field/value:
  - BLANK: If you want to blank a field instead of remove it, use this option. This is the default action.
  - KEEP: implies that the value should not be replaced, removed, or blanked.
  - REPLACE: implies that the value should be replaced by a string, or a variable in the format `var:FieldName`.
- - REMOVE: completely remove the field from the dataset.
+ - REMOVE: completely remove the field from the dataset (if not protected by `KEEP` or is in default skip list).
 
 For the above, given that there are conflicting commands, the more conservative is given preference. For example:
 
 ```
-REMOVE > BLANK > REPLACE > JITTER/KEEP/ADD
+KEEP > ADD > REMOVE > BLANK > REPLACE > JITTER
 ``` 
 
-For example, if I add or keep a header, but then also specify to blank or remove it, 
-it will be blanked or removed. If I specify to blank a header and remove it, it will be removed. 
+For example, if I add or keep a header, and then also specify to blank or remove it, 
+it will be kept. If I specify to blank a header and remove it, it will be removed. 
 If I specify to replace a header and blank it, it will be blanked. Most of the time, 
 you won't need to specify remove, because it is the default. If we were to come up with 
 a pretend config file to represent the default, it would look like this:
@@ -282,7 +282,7 @@ JITTER endswith:Date var:jitter
 
 and this is the idea of an `expander`. And expander is an optional filter 
 applied to a header field (the middle value) to select some subset of header 
-values. Currently, we support `startswith`, `endswith`, `contains`, `allexcept`,
+values. Currently, we support `startswith`, `endswith`, `contains`, `except`,
 and `allfields`.
  
 The following examples show what fields are selected based on each filter. For
@@ -336,7 +336,7 @@ ADD ALL func:remove_identifiers
 
 **except**
 
-Akin to all fields, except provide a list of fields that you want to disclude from
+Akin to all fields, except provide a pattern that you want to exclude from
 a global selector. Here are some examples to include all fields except StudyDate 
 or StudyTime.
 
@@ -345,6 +345,13 @@ BLANK except:StudyDate|StudyTime
 BLANK except:StudyTime
 ```
 
+_except_ is checking field names against given pattern, so if you for example set
+```
+REMOVE except:Manufacturer
+```
+
+following fields will be preserved: `Manufacturer` **and** `ManufacturerModelName`
+
 If you are familiar with regular expressions, you'll notice the "|" which 
 means "or" in the regular expression. You are free to write whatever regular
 expression fits your needs to disclude particular fields here.


Test patch:
diff --git a/deid/tests/test_replace_identifiers.py b/deid/tests/test_replace_identifiers.py
index 80f87cc3..5bbed516 100644
--- a/deid/tests/test_replace_identifiers.py
+++ b/deid/tests/test_replace_identifiers.py
@@ -698,6 +698,190 @@ def contains_hibbard(dicom, value, field, item):
         with self.assertRaises(KeyError):
             check4 = parser.dicom["00331019"].value
 
+    def test_remove_all_keep_field_compounding_should_keep(self):
+        """
+        %header
+        REMOVE ALL
+        KEEP StudyDate
+        ADD PatientIdentityRemoved Yes
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "ALL"},
+            {"action": "KEEP", "field": "StudyDate"},
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        self.assertIsNotNone(parser.dicom["StudyDate"])
+        self.assertEqual("20230101", parser.dicom["StudyDate"].value)
+
+    def test_remove_except_field_keep_other_field_compounding_should_keep(self):
+        """
+        %header
+        REMOVE ALL
+        ADD PatientIdentityRemoved Yes
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "except:Manufacturer"},
+            {"action": "KEEP", "field": "StudyDate"},
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        self.assertIsNotNone(parser.dicom["Manufacturer"])
+        self.assertIsNotNone(parser.dicom["ManufacturerModelName"])
+        self.assertIsNotNone(parser.dicom["StudyDate"])
+
+    def test_remove_all_add_field_compounding_should_add(self):
+        """
+        %header
+        REMOVE ALL
+        ADD PatientIdentityRemoved Yes
+        ADD StudyDate 19700101
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "ALL"},
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        self.assertEqual("19700101", parser.dicom["StudyDate"].value)
+
+    def test_remove_all_blank_field_compounding_should_remove(self):
+        """
+        %header
+        REMOVE ALL
+        ADD PatientIdentityRemoved Yes
+        BLANK StudyDate
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "ALL"},
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+            {"action": "BLANK", "field": "StudyDate"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        with self.assertRaises(KeyError):
+            check3 = parser.dicom["StudyDate"].value
+
+    def test_blank_field_keep_field_compounding_should_keep(self):
+        """
+        %header
+        ADD PatientIdentityRemoved Yes
+        BLANK StudyDate
+        KEEP StudyDate
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+            {"action": "BLANK", "field": "StudyDate"},
+            {"action": "KEEP", "field": "StudyDate"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        self.assertEqual("20230101", parser.dicom["StudyDate"].value)
+
+    def test_remove_keep_add_field_compounding_should_add(self):
+        """
+        %header
+        REMOVE ALL
+        KEEP StudyDate
+        ADD StudyDate 19700101
+        ADD PatientIdentityRemoved Yes
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "ALL"},
+            {"action": "KEEP", "field": "StudyDate"},
+            {"action": "ADD", "field": "StudyDate", "value": "19700101"},
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        self.assertEqual("19700101", parser.dicom["StudyDate"].value)
+
+    def test_remove_field_keep_same_field_compounding_should_keep(self):
+        """
+        %header
+        REMOVE StudyDate
+        KEEP StudyDate
+        ADD PatientIdentityRemoved Yes
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "StudyDate"},
+            {"action": "KEEP", "field": "StudyDate"},
+            {"action": "ADD", "field": "PatientIdentityRemoved", "value": "Yes"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertEqual("Yes", parser.dicom["PatientIdentityRemoved"].value)
+        self.assertIsNotNone(parser.dicom["PixelData"])
+        self.assertEqual("20230101", parser.dicom["StudyDate"].value)
+
+    def test_remove_except_is_acting_as_substring(self):
+        """
+        %header
+        REMOVE except:Manufacturer
+        """
+        dicom_file = get_file(self.dataset)
+
+        actions = [
+            {"action": "REMOVE", "field": "except:Manufacturer"},
+        ]
+        recipe = create_recipe(actions)
+
+        parser = DicomParser(dicom_file, recipe=recipe)
+        parser.parse()
+
+        self.assertIsNotNone(parser.dicom["Manufacturer"])
+        self.assertIsNotNone(parser.dicom["ManufacturerModelName"])
+
     def test_strip_sequences(self):
         """
         Testing strip sequences: Checks to ensure that the strip_sequences removes all tags of type